[PATCH STABLE V2] localrepo: discard objects in _filecache at transaction failure (issue4876)

[FUJIWARA Katsunori]

At Sat, 24 Oct 2015 09:32:18 +0900,
FUJIWARA Katsunori wrote:
> 
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy at 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 at selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp