Patchwork [remotenames] remotenames: use atomictemp when writing files

login
register
mail settings
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 - April 14, 2016, 10:19 p.m.
# 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.
Sean Farley - April 14, 2016, 10:34 p.m.
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.
Jun Wu - April 15, 2016, 4:09 p.m.
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.
Sean Farley - April 15, 2016, 8:51 p.m.
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!
Matt Mackall - April 16, 2016, 6:34 p.m.
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.
Jun Wu - April 16, 2016, 6:58 p.m.
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):