Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On 10/13/2016 05:39 PM, Pierre-Yves David wrote: On 10/13/2016 05:34 PM, Yuya Nishihara wrote: On Thu, 13 Oct 2016 17:28:02 +0200, Mads Kiilerich wrote: On 10/13/2016 05:07 PM, Pierre-Yves David wrote: On 10/13/2016 03:21 PM, Mads Kiilerich wrote: On 10/13/2016 01:53 PM, Pierre-Yves David wrote: # HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. Hmm. *If* the user has write access to the repo and *could* lock the repo, then it seems reasonable that it waits for the lock and does the right thing. It would be unfortunate to bail out early and happily continue to expose the less optimal state that read only users might have to deal with. The change introduced by this changeset make the cache in line with how most of other cache works in Mercurial. A part of the problem here might be that it is unclear to me what happens with wait=False. I don't remember the details: will it continue without locking or will it raise? It would be nice to get that clarified in the localrepo docstrings. LockHeld would be raised. So we'll need to catch LockError if wait=False. And we actually only catch LockUnavailable, lets drop patch 1 for now (patch 2 is still valid) So looking more into this it appears that: - This function is always run for every reposetup(), (irk) - If the cache seems up to date the function end early, (okay) - If the content within the locking scope is not run, the repository have an inconsistent state and can misbehave (irk) My current strategy is to slowly back away from this code and leave it in its current state. Cheers, -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On 10/13/2016 05:34 PM, Yuya Nishihara wrote: On Thu, 13 Oct 2016 17:28:02 +0200, Mads Kiilerich wrote: On 10/13/2016 05:07 PM, Pierre-Yves David wrote: On 10/13/2016 03:21 PM, Mads Kiilerich wrote: On 10/13/2016 01:53 PM, Pierre-Yves David wrote: # HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. Hmm. *If* the user has write access to the repo and *could* lock the repo, then it seems reasonable that it waits for the lock and does the right thing. It would be unfortunate to bail out early and happily continue to expose the less optimal state that read only users might have to deal with. The change introduced by this changeset make the cache in line with how most of other cache works in Mercurial. A part of the problem here might be that it is unclear to me what happens with wait=False. I don't remember the details: will it continue without locking or will it raise? It would be nice to get that clarified in the localrepo docstrings. LockHeld would be raised. So we'll need to catch LockError if wait=False. And we actually only catch LockUnavailable, lets drop patch 1 for now (patch 2 is still valid) -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On Thu, 13 Oct 2016 17:28:02 +0200, Mads Kiilerich wrote: > On 10/13/2016 05:07 PM, Pierre-Yves David wrote: > > On 10/13/2016 03:21 PM, Mads Kiilerich wrote: > >> On 10/13/2016 01:53 PM, Pierre-Yves David wrote: > >>> # HG changeset patch > >>> # User Pierre-Yves David > >>> # Date 1476359131 -7200 > >>> # Thu Oct 13 13:45:31 2016 +0200 > >>> # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 > >>> # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b > >>> eol: do not wait on lack when writing cache > >>> > >>> The cache writing process is properly catching and handling the case > >>> where the > >>> lock is unavailable. However, it fails to specify the lock can failed > >>> to be > >>> acquired when requesting it. This is now fixed. > >> > >> Hmm. > >> > >> *If* the user has write access to the repo and *could* lock the repo, > >> then it seems reasonable that it waits for the lock and does the right > >> thing. It would be unfortunate to bail out early and happily continue to > >> expose the less optimal state that read only users might have to deal > >> with. > > > > The change introduced by this changeset make the cache in line with > > how most of other cache works in Mercurial. > > A part of the problem here might be that it is unclear to me what > happens with wait=False. I don't remember the details: will it continue > without locking or will it raise? It would be nice to get that clarified > in the localrepo docstrings. LockHeld would be raised. So we'll need to catch LockError if wait=False. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On 10/13/2016 05:07 PM, Pierre-Yves David wrote: On 10/13/2016 03:21 PM, Mads Kiilerich wrote: On 10/13/2016 01:53 PM, Pierre-Yves David wrote: # HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. Hmm. *If* the user has write access to the repo and *could* lock the repo, then it seems reasonable that it waits for the lock and does the right thing. It would be unfortunate to bail out early and happily continue to expose the less optimal state that read only users might have to deal with. The change introduced by this changeset make the cache in line with how most of other cache works in Mercurial. A part of the problem here might be that it is unclear to me what happens with wait=False. I don't remember the details: will it continue without locking or will it raise? It would be nice to get that clarified in the localrepo docstrings. Also, this "cache" is not so much a "store values so we don't have to compute them again" cache. It is more about recording how .hgeol looked like when file content from the repo was "cached" in the working directory. Without this, we have to invalidate the dirstate more often. The lock don't really have time out so simple read only command could hold themself forever if make this call blocking. Why not "really"? There is the 10 minutes timeout? Given than eol is probably not going to be user on pulling Sorry, I don't understand that sentence. /Mads ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On 10/13/2016 03:21 PM, Mads Kiilerich wrote: On 10/13/2016 01:53 PM, Pierre-Yves David wrote: # HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. Hmm. *If* the user has write access to the repo and *could* lock the repo, then it seems reasonable that it waits for the lock and does the right thing. It would be unfortunate to bail out early and happily continue to expose the less optimal state that read only users might have to deal with. The change introduced by this changeset make the cache in line with how most of other cache works in Mercurial. The lock don't really have time out so simple read only command could hold themself forever if make this call blocking. Given than eol is probably not going to be user on pulling I thus don't think this change would make it less reliable ... and I don't see it solving a real problem. The idea is to align it with what other cache do (reducing the chance of simple command working). (The next change for proper release is however +1.) yep, next one is a must go ☺ -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 1 of 2] eol: do not wait on lack when writing cache
On 10/13/2016 01:53 PM, Pierre-Yves David wrote: # HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. Hmm. *If* the user has write access to the repo and *could* lock the repo, then it seems reasonable that it waits for the lock and does the right thing. It would be unfortunate to bail out early and happily continue to expose the less optimal state that read only users might have to deal with. I thus don't think this change would make it less reliable ... and I don't see it solving a real problem. (The next change for proper release is however +1.) /Mads diff --git a/hgext/eol.py b/hgext/eol.py --- a/hgext/eol.py +++ b/hgext/eol.py @@ -335,7 +335,7 @@ def reposetup(ui, repo): wlock = None try: -wlock = self.wlock() +wlock = self.wlock(wait=False) for f in self.dirstate: if self.dirstate[f] != 'n': continue ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH 1 of 2] eol: do not wait on lack when writing cache
# HG changeset patch # User Pierre-Yves David # Date 1476359131 -7200 # Thu Oct 13 13:45:31 2016 +0200 # Node ID 88cc944830d0c1895e527d6ca13687f1d5e1c785 # Parent 747e546c561fbf34d07cd30013eaf42b0190bb3b eol: do not wait on lack when writing cache The cache writing process is properly catching and handling the case where the lock is unavailable. However, it fails to specify the lock can failed to be acquired when requesting it. This is now fixed. diff --git a/hgext/eol.py b/hgext/eol.py --- a/hgext/eol.py +++ b/hgext/eol.py @@ -335,7 +335,7 @@ def reposetup(ui, repo): wlock = None try: -wlock = self.wlock() +wlock = self.wlock(wait=False) for f in self.dirstate: if self.dirstate[f] != 'n': continue ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
