| Submitter | Jun Wu |
|---|---|
| Date | April 14, 2016, 10:19 p.m. |
| Message ID | <d3c9e3afef2973f16df7.1460672378@x1c> |
| Download | mbox | patch |
| Permalink | /patch/14635/ |
| State | Accepted |
| Headers | show |
Comments
Jun Wu <quark@fb.com> writes: > # HG changeset patch > # User Jun Wu <quark@fb.com> > # Date 1460672160 -3600 > # Thu Apr 14 23:16:00 2016 +0100 > # Node ID d3c9e3afef2973f16df7778991cee2ba4990a6bb > # Parent 670f94e39c01fd55663b0e1c7b829f78f54d79ac > remotenames: use atomictemp when writing files > > Before this patch, it is possible to have a race condition that a process can > see an incomplete .hg/remotenames and may crash like: Awesome, queued this.
On 04/14/2016 11:34 PM, Sean Farley wrote:
> Awesome, queued this.
FYI. The team had the issue just reported it disappeared after applying the
patch. Although I'm still unclear about why repo wlock + close syscall don't
work, but it seems it was fixed.
Jun Wu <quark@fb.com> writes: > On 04/14/2016 11:34 PM, Sean Farley wrote: >> Awesome, queued this. > > FYI. The team had the issue just reported it disappeared after applying the > patch. Although I'm still unclear about why repo wlock + close syscall don't > work, but it seems it was fixed. Well, I'm glad it worked!
On Fri, 2016-04-15 at 17:09 +0100, Jun Wu wrote: > On 04/14/2016 11:34 PM, Sean Farley wrote: > > > > Awesome, queued this. > FYI. The team had the issue just reported it disappeared after applying the > patch. Although I'm still unclear about why repo wlock + close syscall don't The wlock does not lock out other readers, only writers. So a >4k write could be result in a reader seeing an incomplete file if it calls read() in the middle of the write(). close() itself has no effect on the state visible on local filesystems[1]: data becomes visible during the write() call. [1] But on NFS: http://nfs.sourceforge.net/#faq_a8 -- Mathematics is the supreme nostalgia of our time.
On 04/16/2016 07:34 PM, Matt Mackall wrote:
> The wlock does not lock out other readers, only writers.
Ah, very clear. Thanks! I confused wlock with LOCK_EX of flock.
Patch
diff --git a/remotenames.py b/remotenames.py --- a/remotenames.py +++ b/remotenames.py @@ -450,7 +450,7 @@ if not repo.vfs.isfile('hgrc'): raise error.Abort(_("could not find hgrc file")) oldhgrc = repo.vfs.read('hgrc').splitlines(True) - f = repo.vfs('hgrc', 'w') + f = repo.vfs('hgrc', 'w', atomictemp=True) for line in oldhgrc: if '[paths]' in line: foundpaths = True @@ -468,7 +468,7 @@ oldhgrc = [] if repo.vfs.isfile("hgrc"): oldhgrc = repo.vfs.read('hgrc').splitlines(True) - f = repo.vfs('hgrc', 'w') + f = repo.vfs('hgrc', 'w', atomictemp=True) done = False for line in oldhgrc: if '[paths]' in line: @@ -1316,7 +1316,7 @@ olddata = set(readremotenames(repo)) oldbooks = {} - f = vfs('remotenames', 'w') + f = vfs('remotenames', 'w', atomictemp=True) # only update the given 'remote path'; iterate over # old data and re-save it @@ -1378,7 +1378,7 @@ def writedistancecache(repo, distance): try: vfs = shareawarevfs(repo) - f = vfs('cache/distance', 'w') + f = vfs('cache/distance', 'w', atomictemp=True) for k, v in distance.iteritems(): f.write('%s %d %d\n' % (k, v[0], v[1])) except (IOError, OSError):
# HG changeset patch # User Jun Wu <quark@fb.com> # Date 1460672160 -3600 # Thu Apr 14 23:16:00 2016 +0100 # Node ID d3c9e3afef2973f16df7778991cee2ba4990a6bb # Parent 670f94e39c01fd55663b0e1c7b829f78f54d79ac remotenames: use atomictemp when writing files Before this patch, it is possible to have a race condition that a process can see an incomplete .hg/remotenames and may crash like: Traceback (last part): File ".../remotenames.py", line 233, in _load for node, nametype, remote, rname in readremotenames(repo): File ".../remotenames.py", line 1245, in readremotenames node, name = line.split(' ', 1) ValueError: need more than 1 value to unpack This patch fixes it and avoids similar issues by using atomictemp for all file writes.