D6861: fastannotate: remove support for flock() locking

[durin42 (Augie Fackler)]

durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We've seen enough weirdness in CI with flock for remotefilelog that
  I'm now of the opinion we should just stop using flock() everywhere
  until someone has a concrete need for the extra performance *and* a
  way to only use it when safe (even if that's just default-to-off.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6861

AFFECTED FILES
  hgext/fastannotate/__init__.py
  hgext/fastannotate/context.py

CHANGE DETAILS

diff --git a/hgext/fastannotate/context.py b/hgext/fastannotate/context.py
--- a/hgext/fastannotate/context.py
+++ b/hgext/fastannotate/context.py
@@ -759,21 +759,6 @@
     def lock(self):
         return lockmod.lock(self._repo.vfs, self._vfspath + '.lock')
 
-    @contextlib.contextmanager
-    def _lockflock(self):
-        """the same as 'lock' but use flock instead of lockmod.lock, to avoid
-        creating temporary symlinks."""
-        import fcntl
-        lockpath = self.linelogpath
-        util.makedirs(os.path.dirname(lockpath))
-        lockfd = os.open(lockpath, os.O_RDONLY | os.O_CREAT, 0o664)
-        fcntl.flock(lockfd, fcntl.LOCK_EX)
-        try:
-            yield
-        finally:
-            fcntl.flock(lockfd, fcntl.LOCK_UN)
-            os.close(lockfd)
-
     @property
     def revmappath(self):
         return self._repo.vfs.join(self._vfspath + '.m')
diff --git a/hgext/fastannotate/__init__.py b/hgext/fastannotate/__init__.py
--- a/hgext/fastannotate/__init__.py
+++ b/hgext/fastannotate/__init__.py
@@ -62,13 +62,6 @@
     # the server. (default: 10)
     clientfetchthreshold = 10
 
-    # use flock instead of the file existence lock
-    # flock may not work well on some network filesystems, but they avoid
-    # creating and deleting files frequently, which is faster when updating
-    # the annotate cache in batch. if you have issues with this option, set it
-    # to False. (default: True if flock is supported, False otherwise)
-    useflock = True
-
     # for "fctx" mode, always follow renames regardless of command line option.
     # this is a BC with the original command but will reduced the space needed
     # for annotate cache, and is useful for client-server setup since the
@@ -100,8 +93,6 @@
 #
 # * rename the config knob for updating the local cache from a remote server
 #
-# * move `flock` based locking to a common area
-#
 # * revise wireprotocol for sharing annotate files
 #
 # * figure out a sensible default for `mainbranch` (with the caveat
@@ -114,7 +105,6 @@
 
 from mercurial.i18n import _
 from mercurial import (
-    configitems,
     error as hgerror,
     localrepo,
     registrar,
@@ -122,7 +112,6 @@
 
 from . import (
     commands,
-    context,
     protocol,
 )
 
@@ -139,7 +128,6 @@
 
 configitem('fastannotate', 'modes', default=['fastannotate'])
 configitem('fastannotate', 'server', default=False)
-configitem('fastannotate', 'useflock', default=configitems.dynamicdefault)
 configitem('fastannotate', 'client', default=False)
 configitem('fastannotate', 'unfilteredrepo', default=True)
 configitem('fastannotate', 'defaultformat', default=['number'])
@@ -151,14 +139,6 @@
 configitem('fastannotate', 'serverbuildondemand', default=True)
 configitem('fastannotate', 'remotepath', default='default')
 
-def _flockavailable():
-    try:
-        import fcntl
-        fcntl.flock
-    except (AttributeError, ImportError):
-        return False
-    else:
-        return True
 
 def uisetup(ui):
     modes = set(ui.configlist('fastannotate', 'modes'))
@@ -180,9 +160,6 @@
     if ui.configbool('fastannotate', 'server'):
         protocol.serveruisetup(ui)
 
-    if ui.configbool('fastannotate', 'useflock', _flockavailable()):
-        context.pathhelper.lock = context.pathhelper._lockflock
-
 def extsetup(ui):
     # fastannotate has its own locking, without depending on repo lock
     # TODO: avoid mutating this unless the specific repo has it enabled



To: durin42, #hg-reviewers
Cc: mercurial-devel