Submitter | Matt Mackall |
---|---|
Date | April 28, 2016, 9:54 p.m. |
Message ID | <29c1fba34f3426a21ef6.1461880469@ruin.waste.org> |
Download | mbox | patch |
Permalink | /patch/14828/ |
State | Accepted |
Delegated to: | Yuya Nishihara |
Headers | show |
Comments
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.
Patch
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() 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()