Patchwork [3,of,4,stable] repoview: ignore unwritable hidden cache

# HG changeset patch
# User Matt Mackall <mpm@selenic.com>
# Date 1461878778 18000
#      Thu Apr 28 16:26:18 2016 -0500
# Branch stable
# Node ID 29c1fba34f3426a21ef692ae38bdfd5b49599c6a
# Parent  369f16a0fbd46c826ee75eff85f7db51099e0d17
repoview: ignore unwritable hidden cache

The atomictemp.close() file attempts to do a rename, which can fail.
Moving the close inside the exception handler fixes it.

This doesn't fit well with the with: pattern, as it's the finalizer
that's failing.

On 28/04/2016 22:54, Matt Mackall wrote:
> # HG changeset patch
> # User Matt Mackall <mpm@selenic.com>
> # Date 1461878778 18000
> #      Thu Apr 28 16:26:18 2016 -0500
> # Branch stable
> # Node ID 29c1fba34f3426a21ef692ae38bdfd5b49599c6a
> # Parent  369f16a0fbd46c826ee75eff85f7db51099e0d17
> repoview: ignore unwritable hidden cache
>
> The atomictemp.close() file attempts to do a rename, which can fail.
> Moving the close inside the exception handler fixes it.
>
> This doesn't fit well with the with: pattern, as it's the finalizer
> that's failing.
>
> diff -r 369f16a0fbd4 -r 29c1fba34f34 mercurial/repoview.py
> --- a/mercurial/repoview.py	Thu Apr 28 15:40:43 2016 -0500
> +++ b/mercurial/repoview.py	Thu Apr 28 16:26:18 2016 -0500
> @@ -130,13 +130,12 @@
>           newhash = cachehash(repo, hideable)
>           fh = repo.vfs.open(cachefile, 'w+b', atomictemp=True)
>           _writehiddencache(fh, newhash, hidden)
> +        fh.close()

What closes fh if there's an exception in _writehiddencache?

I wonder if we're actually exposing a latent bug in atomictemp here?

>       except (IOError, OSError):
>           repo.ui.debug('error writing hidden changesets cache\n')
>       except error.LockHeld:
>           repo.ui.debug('cannot obtain lock to write hidden changesets cache\n')
>       finally:
> -        if fh:
> -            fh.close()
>           if wlock:
>               wlock.release()
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=H8VPlMTybBojOJqODJ-f3lJBCAbWjqmIICZb_k__JSM&s=9XYrCXP_31LbPhR6GJfMIz-Wr9uXrvuxMExqbkwP5Lk&e=
>

On Fri, 2016-04-29 at 15:22 +0100, Simon Farnsworth wrote:
> On 28/04/2016 22:54, Matt Mackall wrote:
> > 
> > # HG changeset patch
> > # User Matt Mackall <mpm@selenic.com>
> > # Date 1461878778 18000
> > #      Thu Apr 28 16:26:18 2016 -0500
> > # Branch stable
> > # Node ID 29c1fba34f3426a21ef692ae38bdfd5b49599c6a
> > # Parent  369f16a0fbd46c826ee75eff85f7db51099e0d17
> > repoview: ignore unwritable hidden cache
> > 
> > The atomictemp.close() file attempts to do a rename, which can fail.
> > Moving the close inside the exception handler fixes it.
> > 
> > This doesn't fit well with the with: pattern, as it's the finalizer
> > that's failing.
> > 
> > diff -r 369f16a0fbd4 -r 29c1fba34f34 mercurial/repoview.py
> > --- a/mercurial/repoview.py	Thu Apr 28 15:40:43 2016 -0500
> > +++ b/mercurial/repoview.py	Thu Apr 28 16:26:18 2016 -0500
> > @@ -130,13 +130,12 @@
> >           newhash = cachehash(repo, hideable)
> >           fh = repo.vfs.open(cachefile, 'w+b', atomictemp=True)
> >           _writehiddencache(fh, newhash, hidden)
> > +        fh.close()
> What closes fh if there's an exception in _writehiddencache?

The "file handle" is actually an atomictempfile and its close() method does more
than close an OS-level file descriptor. It also does a rename. And now that I
look at it, that rename is NOT something we want to happen if we have some sort
of exception: the current code was actually buggy in this regard.

As it happens, atomictempfile has a __del__ method that will probably delete the
temp file (self.discard()) rather than rename it if we never close it properly.
If we grow a finalizer, it should call discard() rather than close().

Our priorities here are something like:

- don't abort hg because someone else owns the hidden cache
- shut the hell up about it too
- don't finalize an aborted operation
- clean up the temp file
- don't leak an OS-level file descriptor

I think this patch hits all those points, but I won't lose any sleep over the
last two.

> I wonder if we're actually exposing a latent bug in atomictemp here?

I'd say no. It's more that the atomictempfile pattern mismatches our usual
expectations: close is not something that should be done unconditionally.

After the freeze, it should probably grow an __exit__ method and maybe we should
alias close() to finalize() for clarity.

-- 
Mathematics is the supreme nostalgia of our time.