Patchwork [5,of,5] fsmonitor: don't write out state if identity has changed (issue5581)

login
register
mail settings
Submitter Siddharth Agarwal
Date June 12, 2017, 10:36 p.m.
Message ID <f515d7f7f13b07aeb39b.1497306999@devvm31800.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21351/
State Accepted
Headers show

Comments

Siddharth Agarwal - June 12, 2017, 10:36 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1497306871 25200
#      Mon Jun 12 15:34:31 2017 -0700
# Node ID f515d7f7f13b07aeb39b4ae39ea6222eac01a3f2
# Parent  1b02cda9518bbda5ad4a08bbad5b3c3666646dbe
fsmonitor: don't write out state if identity has changed (issue5581)

Inspired by the dirstate fix in dc7efa2826e4, this should fix any race
conditions with the fsmonitor state changing from underneath.

Since we now grab the wlock for any non-invalidate writes, the only situation
this appears to happen in is with a concurrent invalidation. Test that.
Siddharth Agarwal - June 12, 2017, 11:25 p.m.
On 6/12/17 3:36 PM, Siddharth Agarwal wrote:
> +        # Read the identity from the file on disk rather than from the open file
> +        # pointer below, because the latter is actually a brand new file.
> +        identity = util.filestat.frompath(self._vfs.join('fsmonitor.state'))
> +        if identity != self._identity:
> +            self._ui.debug('skip updating fsmonitor.state: identity mismatch\n')
> +            return
> +        else:
> +            self._ui.debug('identity new: %s %s old: %s %s' % (identity, identity.stat, self._identity, self._identity.stat))


Whoops, left these last two lines in by accident. Happy to send a V2, or 
if someone can fix it inflight that would be great.
Augie Fackler - June 13, 2017, 2:10 p.m.
On Mon, Jun 12, 2017 at 03:36:39PM -0700, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1497306871 25200
> #      Mon Jun 12 15:34:31 2017 -0700
> # Node ID f515d7f7f13b07aeb39b4ae39ea6222eac01a3f2
> # Parent  1b02cda9518bbda5ad4a08bbad5b3c3666646dbe
> fsmonitor: don't write out state if identity has changed (issue5581)

queued, thanks

Patch

diff --git a/hgext/fsmonitor/state.py b/hgext/fsmonitor/state.py
--- a/hgext/fsmonitor/state.py
+++ b/hgext/fsmonitor/state.py
@@ -13,7 +13,10 @@  import socket
 import struct
 
 from mercurial.i18n import _
-from mercurial import pathutil
+from mercurial import (
+    pathutil,
+    util,
+)
 
 _version = 4
 _versionformat = ">I"
@@ -24,6 +27,7 @@  class state(object):
         self._ui = repo.ui
         self._rootdir = pathutil.normasprefix(repo.root)
         self._lastclock = None
+        self._identity = util.filestat(None)
 
         self.mode = self._ui.config('fsmonitor', 'mode', default='on')
         self.walk_on_invalidate = self._ui.configbool(
@@ -35,10 +39,13 @@  class state(object):
         try:
             file = self._vfs('fsmonitor.state', 'rb')
         except IOError as inst:
+            self._identity = util.filestat(None)
             if inst.errno != errno.ENOENT:
                 raise
             return None, None, None
 
+        self._identity = util.filestat.fromfp(file)
+
         versionbytes = file.read(4)
         if len(versionbytes) < 4:
             self._ui.log(
@@ -90,8 +97,18 @@  class state(object):
             self.invalidate()
             return
 
+        # Read the identity from the file on disk rather than from the open file
+        # pointer below, because the latter is actually a brand new file.
+        identity = util.filestat.frompath(self._vfs.join('fsmonitor.state'))
+        if identity != self._identity:
+            self._ui.debug('skip updating fsmonitor.state: identity mismatch\n')
+            return
+        else:
+            self._ui.debug('identity new: %s %s old: %s %s' % (identity, identity.stat, self._identity, self._identity.stat))
+
         try:
-            file = self._vfs('fsmonitor.state', 'wb', atomictemp=True)
+            file = self._vfs('fsmonitor.state', 'wb', atomictemp=True,
+                checkambig=True)
         except (IOError, OSError):
             self._ui.warn(_("warning: unable to write out fsmonitor state\n"))
             return
@@ -111,6 +128,7 @@  class state(object):
         except OSError as inst:
             if inst.errno != errno.ENOENT:
                 raise
+        self._identity = util.filestat(None)
 
     def setlastclock(self, clock):
         self._lastclock = clock
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1519,6 +1519,11 @@  class filestat(object):
             stat = None
         return cls(stat)
 
+    @classmethod
+    def fromfp(cls, fp):
+        stat = os.fstat(fp.fileno())
+        return cls(stat)
+
     __hash__ = object.__hash__
 
     def __eq__(self, old):
diff --git a/tests/test-dirstate-race.t b/tests/test-dirstate-race.t
--- a/tests/test-dirstate-race.t
+++ b/tests/test-dirstate-race.t
@@ -160,6 +160,34 @@  treated differently in _checklookup() ac
 
   $ rm b
 
+#if fsmonitor
+
+Create fsmonitor state.
+
+  $ hg status
+  $ f --type .hg/fsmonitor.state
+  .hg/fsmonitor.state: file
+
+Test that invalidating fsmonitor state in the middle (which doesn't require the
+wlock) causes the fsmonitor update to be skipped.
+hg debugrebuilddirstate ensures that the dirstaterace hook will be called, but
+it also invalidates the fsmonitor state. So back it up and restore it.
+
+  $ mv .hg/fsmonitor.state .hg/fsmonitor.state.tmp
+  $ hg debugrebuilddirstate
+  $ mv .hg/fsmonitor.state.tmp .hg/fsmonitor.state
+
+  $ cat > $TESTTMP/dirstaterace.sh <<EOF
+  > rm .hg/fsmonitor.state
+  > EOF
+
+  $ hg status --config extensions.dirstaterace=$TESTTMP/dirstaterace.py --debug
+  skip updating fsmonitor.state: identity mismatch
+  $ f .hg/fsmonitor.state
+  .hg/fsmonitor.state: file not found
+
+#endif
+
 Set up a rebase situation for issue5581.
 
   $ echo c2 > a