| Submitter | Katsunori FUJIWARA |
|---|---|
| Date | Oct. 24, 2015, 12:32 a.m. |
| Message ID | <590f5ac256895fb857da.1445646738@feefifofum> |
| Download | mbox | patch |
| Permalink | /patch/11227/ |
| State | Superseded |
| Headers | show |
Comments
At Sat, 24 Oct 2015 09:32:18 +0900, FUJIWARA Katsunori wrote: > > # HG changeset patch > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > # Date 1445644900 -32400 > # Sat Oct 24 09:01:40 2015 +0900 > # Branch stable > # Node ID 590f5ac256895fb857dafabdefd6863b82679583 > # Parent 39dbf495880b8a439d912091109427d27a7e616a > localrepo: discard objects in _filecache at transaction failure (issue4876) Please ignore this posting, because this description is a little ambiguous. I'll post revised one, again. > 'repo.invalidate()' deletes 'filecache'-ed properties by > 'filecache.__delete__()' below via 'delattr(unfiltered, k)'. But > cached objects are still kept in 'repo._filecache'. > > def __delete__(self, obj): > try: > del obj.__dict__[self.name] > except KeyError: > raise AttributeError(self.name) > > If 'repo' object is reused even after failure of command execution, > referring 'filecache'-ed property may reuse one kept in > 'repo._filecache', even if reloading from a file is expected. > > Executing command sequence on command server is a typical case of this > situation (5c0f5db65c6b also tried to fix this issue). For example: > > 1. start a command execution > > 2. 'changelog.delayupdate()' is invoked in a transaction scope > > This replaces own 'opener' by '_divertopener()' for additional > accessing to '00changelog.i.a' > > 3. transaction is aborted, and command (1) execution is ended > > After 'repo.invalidate()' at releasing store lock, changelog > object above (= 'opener' of it is still replaced) is deleted from > 'repo.__dict__', but still kept in 'repo._filecache' > > 4. start next command execution with same 'repo' > > 5. referring 'repo.changelog' may reuse changelog object kept in > 'repo._filecache' according to timestamp > > Then, "No such file or directory" error occurs for > '00changelog.i.a', which is already removed at (3) > > This patch discards objects in '_filecache' at transaction failure. > > 'repo.invalidate()' at failure of "hg qpush" is removed, because now > it is redundant. > > Changes in 'invalidate()' can't be simplified by 'self._filecache = > {}', because 'invalidate()' itself should keep 'dirstate' in it. > > This patch doesn't make 'repo.invalidate()' always discard objects in > '_filecache', because 'repo.invalidate()' is invoked also at unlocking > store lock. > > - "always discard objects in filecache at unlocking" may cause > serious performance problem for subsequent procedures at normal > execution > > - but it is impossible to "discard objects in filecache at unlocking > only at failure", because 'releasefn' of lock can't know whether a > lock scope is terminated normally or not > > BTW, using "with" statement described in PEP343 for lock may > resolve this ? > > IMHO, fundamental cause of this issue seems that truncation of > '00changelog.i' occurs at failure of transaction, even though newly > added revisions exist only in '00changelog.i.a' (aka "pending > file"). This truncation implies useless updating 'st_mtime' of > '00changelog.i', even though size of '00changelog.i' isn't changed by > truncation itself. > > This behavior will be fixed by dropping '00changelog.i' at aborting > from the list of files to be truncated in transaction after code > freeze. > > diff --git a/hgext/mq.py b/hgext/mq.py > --- a/hgext/mq.py > +++ b/hgext/mq.py > @@ -847,7 +847,6 @@ > try: > tr.abort() > finally: > - repo.invalidate() > self.invalidate() > raise > finally: > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > --- a/mercurial/localrepo.py > +++ b/mercurial/localrepo.py > @@ -1014,6 +1014,8 @@ > # out) in this transaction > repo.vfs.rename('journal.dirstate', 'dirstate') > > + repo.invalidate(clearfilecache=True) > + > tr = transaction.transaction(rp, self.svfs, vfsmap, > "journal", > "undo", > @@ -1205,13 +1207,15 @@ > pass > delattr(self.unfiltered(), 'dirstate') > > - def invalidate(self): > + def invalidate(self, clearfilecache=False): > unfiltered = self.unfiltered() # all file caches are stored unfiltered > - for k in self._filecache: > + for k in self._filecache.keys(): > # dirstate is invalidated separately in invalidatedirstate() > if k == 'dirstate': > continue > > + if clearfilecache: > + del self._filecache[k] > try: > delattr(unfiltered, k) > except AttributeError: > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > https://selenic.com/mailman/listinfo/mercurial-devel ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
Patch
diff --git a/hgext/mq.py b/hgext/mq.py --- a/hgext/mq.py +++ b/hgext/mq.py @@ -847,7 +847,6 @@ try: tr.abort() finally: - repo.invalidate() self.invalidate() raise finally: diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -1014,6 +1014,8 @@ # out) in this transaction repo.vfs.rename('journal.dirstate', 'dirstate') + repo.invalidate(clearfilecache=True) + tr = transaction.transaction(rp, self.svfs, vfsmap, "journal", "undo", @@ -1205,13 +1207,15 @@ pass delattr(self.unfiltered(), 'dirstate') - def invalidate(self): + def invalidate(self, clearfilecache=False): unfiltered = self.unfiltered() # all file caches are stored unfiltered - for k in self._filecache: + for k in self._filecache.keys(): # dirstate is invalidated separately in invalidatedirstate() if k == 'dirstate': continue + if clearfilecache: + del self._filecache[k] try: delattr(unfiltered, k) except AttributeError:
# HG changeset patch # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> # Date 1445644900 -32400 # Sat Oct 24 09:01:40 2015 +0900 # Branch stable # Node ID 590f5ac256895fb857dafabdefd6863b82679583 # Parent 39dbf495880b8a439d912091109427d27a7e616a localrepo: discard objects in _filecache at transaction failure (issue4876) 'repo.invalidate()' deletes 'filecache'-ed properties by 'filecache.__delete__()' below via 'delattr(unfiltered, k)'. But cached objects are still kept in 'repo._filecache'. def __delete__(self, obj): try: del obj.__dict__[self.name] except KeyError: raise AttributeError(self.name) If 'repo' object is reused even after failure of command execution, referring 'filecache'-ed property may reuse one kept in 'repo._filecache', even if reloading from a file is expected. Executing command sequence on command server is a typical case of this situation (5c0f5db65c6b also tried to fix this issue). For example: 1. start a command execution 2. 'changelog.delayupdate()' is invoked in a transaction scope This replaces own 'opener' by '_divertopener()' for additional accessing to '00changelog.i.a' 3. transaction is aborted, and command (1) execution is ended After 'repo.invalidate()' at releasing store lock, changelog object above (= 'opener' of it is still replaced) is deleted from 'repo.__dict__', but still kept in 'repo._filecache' 4. start next command execution with same 'repo' 5. referring 'repo.changelog' may reuse changelog object kept in 'repo._filecache' according to timestamp Then, "No such file or directory" error occurs for '00changelog.i.a', which is already removed at (3) This patch discards objects in '_filecache' at transaction failure. 'repo.invalidate()' at failure of "hg qpush" is removed, because now it is redundant. Changes in 'invalidate()' can't be simplified by 'self._filecache = {}', because 'invalidate()' itself should keep 'dirstate' in it. This patch doesn't make 'repo.invalidate()' always discard objects in '_filecache', because 'repo.invalidate()' is invoked also at unlocking store lock. - "always discard objects in filecache at unlocking" may cause serious performance problem for subsequent procedures at normal execution - but it is impossible to "discard objects in filecache at unlocking only at failure", because 'releasefn' of lock can't know whether a lock scope is terminated normally or not BTW, using "with" statement described in PEP343 for lock may resolve this ? IMHO, fundamental cause of this issue seems that truncation of '00changelog.i' occurs at failure of transaction, even though newly added revisions exist only in '00changelog.i.a' (aka "pending file"). This truncation implies useless updating 'st_mtime' of '00changelog.i', even though size of '00changelog.i' isn't changed by truncation itself. This behavior will be fixed by dropping '00changelog.i' at aborting from the list of files to be truncated in transaction after code freeze.