This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6861

fastannotate: remove support for flock() locking
ClosedPublic

Authored by durin42 on Sep 17 2019, 2:27 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG0152a907f714: fastannotate: remove support for flock() locking
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.)

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Sep 17 2019, 2:27 PM
pulkit accepted this revision.Sep 17 2019, 3:34 PM
This revision is now accepted and ready to land.Sep 17 2019, 3:34 PM
durin42 added a commit: rHG0152a907f714: fastannotate: remove support for flock() locking.Sep 17 2019, 3:57 PM
Closed by commit rHG0152a907f714: fastannotate: remove support for flock() locking (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 16572

hgext/fastannotate/__init__.py

# path to use when connecting to the remote server (default: default) # path to use when connecting to the remote server (default: default)
remotepath = default remotepath = default
# minimal length of the history of a file required to fetch linelog from # minimal length of the history of a file required to fetch linelog from
# the server. (default: 10) # the server. (default: 10)
clientfetchthreshold = 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. # 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 # 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 # for annotate cache, and is useful for client-server setup since the
# server will only provide annotate cache with default options (i.e. with # server will only provide annotate cache with default options (i.e. with
# follow). do not affect "fastannotate" mode. (default: True) # follow). do not affect "fastannotate" mode. (default: True)
forcefollow = True forcefollow = True
# for "fctx" mode, always treat file as text files, to skip the "isbinary" # for "fctx" mode, always treat file as text files, to skip the "isbinary"
# #
# * replace the fastannotate `modes` configuration with a collection # * replace the fastannotate `modes` configuration with a collection
# of booleans. # of booleans.
# #
# * Use the templater instead of bespoke formatting # * Use the templater instead of bespoke formatting
# #
# * rename the config knob for updating the local cache from a remote server # * 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 # * revise wireprotocol for sharing annotate files
# #
# * figure out a sensible default for `mainbranch` (with the caveat # * figure out a sensible default for `mainbranch` (with the caveat
# that we probably also want to figure out a better term than # that we probably also want to figure out a better term than
# `branch`, see above) # `branch`, see above)
# #
# * format changes to the revmap file (maybe use length-encoding # * format changes to the revmap file (maybe use length-encoding
# instead of null-terminated file paths at least?) # instead of null-terminated file paths at least?)
from __future__ import absolute_import from __future__ import absolute_import
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
configitems,
error as hgerror, error as hgerror,
localrepo, localrepo,
registrar, registrar,
) )
from . import ( from . import (
commands, commands,
context,
protocol, protocol,
) )
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or # be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified. # leave the attribute unspecified.
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
cmdtable = commands.cmdtable cmdtable = commands.cmdtable
configtable = {} configtable = {}
configitem = registrar.configitem(configtable) configitem = registrar.configitem(configtable)
configitem('fastannotate', 'modes', default=['fastannotate']) configitem('fastannotate', 'modes', default=['fastannotate'])
configitem('fastannotate', 'server', default=False) configitem('fastannotate', 'server', default=False)
configitem('fastannotate', 'useflock', default=configitems.dynamicdefault)
configitem('fastannotate', 'client', default=False) configitem('fastannotate', 'client', default=False)
configitem('fastannotate', 'unfilteredrepo', default=True) configitem('fastannotate', 'unfilteredrepo', default=True)
configitem('fastannotate', 'defaultformat', default=['number']) configitem('fastannotate', 'defaultformat', default=['number'])
configitem('fastannotate', 'perfhack', default=False) configitem('fastannotate', 'perfhack', default=False)
configitem('fastannotate', 'mainbranch') configitem('fastannotate', 'mainbranch')
configitem('fastannotate', 'forcetext', default=True) configitem('fastannotate', 'forcetext', default=True)
configitem('fastannotate', 'forcefollow', default=True) configitem('fastannotate', 'forcefollow', default=True)
configitem('fastannotate', 'clientfetchthreshold', default=10) configitem('fastannotate', 'clientfetchthreshold', default=10)
configitem('fastannotate', 'serverbuildondemand', default=True) configitem('fastannotate', 'serverbuildondemand', default=True)
configitem('fastannotate', 'remotepath', default='default') configitem('fastannotate', 'remotepath', default='default')
def _flockavailable():
try:
import fcntl
fcntl.flock
except (AttributeError, ImportError):
return False
else:
return True
def uisetup(ui): def uisetup(ui):
modes = set(ui.configlist('fastannotate', 'modes')) modes = set(ui.configlist('fastannotate', 'modes'))
if 'fctx' in modes: if 'fctx' in modes:
modes.discard('hgweb') modes.discard('hgweb')
for name in modes: for name in modes:
if name == 'fastannotate': if name == 'fastannotate':
commands.registercommand() commands.registercommand()
elif name == 'hgweb': elif name == 'hgweb':
from . import support from . import support
support.replacehgwebannotate() support.replacehgwebannotate()
elif name == 'fctx': elif name == 'fctx':
from . import support from . import support
support.replacefctxannotate() support.replacefctxannotate()
commands.wrapdefault() commands.wrapdefault()
else: else:
raise hgerror.Abort(_('fastannotate: invalid mode: %s') % name) raise hgerror.Abort(_('fastannotate: invalid mode: %s') % name)
if ui.configbool('fastannotate', 'server'): if ui.configbool('fastannotate', 'server'):
protocol.serveruisetup(ui) protocol.serveruisetup(ui)
if ui.configbool('fastannotate', 'useflock', _flockavailable()):
context.pathhelper.lock = context.pathhelper._lockflock
def extsetup(ui): def extsetup(ui):
# fastannotate has its own locking, without depending on repo lock # fastannotate has its own locking, without depending on repo lock
# TODO: avoid mutating this unless the specific repo has it enabled # TODO: avoid mutating this unless the specific repo has it enabled
localrepo.localrepository._wlockfreeprefix.add('fastannotate/') localrepo.localrepository._wlockfreeprefix.add('fastannotate/')
def reposetup(ui, repo): def reposetup(ui, repo):
if ui.configbool('fastannotate', 'client'): if ui.configbool('fastannotate', 'client'):
protocol.clientreposetup(ui, repo) protocol.clientreposetup(ui, repo)

hgext/fastannotate/context.py

@property @property
def linelogpath(self): def linelogpath(self):
return self._repo.vfs.join(self._vfspath + '.l') return self._repo.vfs.join(self._vfspath + '.l')
def lock(self): def lock(self):
return lockmod.lock(self._repo.vfs, self._vfspath + '.lock') 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 @property
def revmappath(self): def revmappath(self):
return self._repo.vfs.join(self._vfspath + '.m') return self._repo.vfs.join(self._vfspath + '.m')
@contextlib.contextmanager @contextlib.contextmanager
def annotatecontext(repo, path, opts=defaultopts, rebuild=False): def annotatecontext(repo, path, opts=defaultopts, rebuild=False):
"""context needed to perform (fast) annotate on a file """context needed to perform (fast) annotate on a file