This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1543

repack: move to flock based locking
ClosedPublic

Authored by durham on Nov 29 2017, 1:46 PM.

Details

Reviewers
quark
dsp
Group Reviewers
Restricted Project
Commits
rFBHGX1c05727c4087: repack: move to fcntllock based locking
Summary

Previously repack use the standard Mercurial symlink based locking mechanism. This caused
problems on our laptop users because the symlink locking relies on the host name
and sometimes sometimes their hostname changes due to weird IT issues, which
resulted in locks existing forever and repack never running. The symlink based
locking scheme was also a problem in chroots, where two processes in different
chroots may attempt to repack the same shared cache at the same time.

Switching to an flock based scheme will solve these issues.

Diff Detail

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

Event Timeline

durham created this revision.Nov 29 2017, 1:46 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 29 2017, 1:46 PM
quark accepted this revision.Nov 29 2017, 5:29 PM
quark added a subscriber: quark.

Mostly about renaming to remove flock() ambiguity.

hgext3rd/extutil.py
134–135

I'd rename it to "fcntl lock" in both the class name and comments, since that's different from flock().

See http://chris.improbable.org/2010/12/16/everything-you-never-wanted-to-know-about-file-locking/ for some of their differences.

One of the main differences is fcntl lock will lose after fork while flock() won't. Another seems to be flock() works if the file is opened without write permission.

145

nit: Maybe rename fd to fp since it's a file object, not a file descriptor (int). That is more consistent with D642 (as a response to Yuya's comment on a previous patch).

This revision is now accepted and ready to land.Nov 29 2017, 5:29 PM
akushner added subscribers: ikostia, dsp, akushner.Nov 29 2017, 6:12 PM
akushner added inline comments.
hgext3rd/extutil.py
11

I don't think the fcntl module is available for Windows. @ikostia @dsp

There are portable locking libraries, but not sure if that is overkill

https://github.com/WoLpH/portalocker

quark added a comment.EditedNov 29 2017, 6:22 PM

fcntl is not available in Windows. I think the API is fine and we can replace the implementation later. https://github.com/danburkert/fs2-rs could also be a reasonable cross-platform choice.

akushner added a comment.Nov 29 2017, 6:39 PM

@quark, but won't this change break the Windows release?

dsp requested changes to this revision.Nov 29 2017, 9:24 PM

I am putting this back into the queue as it will break Windows builds, where hg repack currently works. fcntl does not exist on Windows. We can probably get a away with implementing a windows version that relies on the old repo._lock() mechanism.

This revision now requires changes to proceed.Nov 29 2017, 9:24 PM
quark added a comment.Nov 29 2017, 10:06 PM

This was already committed. I'll follow-up adding a Windows code path.

Closed by commit rFBHGX1c05727c4087: repack: move to fcntllock based locking (authored by durham). · Explain WhyNov 29 2017, 10:09 PM
This revision was automatically updated to reflect the committed changes.
ikostia added inline comments.Nov 30 2017, 6:31 AM
hgext3rd/extutil.py
11

Not sure what you mean by 'overkill'. We certainly need a reliable locking strategy on Windows, as we've seen prefetch locks not being cleaned up several times.
Also, thanks for subscribing me.

Revision Contents
Changeset List

Diff 3999

hgext3rd/extutil.py

# extutil.py - useful utility methods for extensions # extutil.py - useful utility methods for extensions
# #
# Copyright 2016 Facebook # Copyright 2016 Facebook
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import errno import errno
import fcntl
akushnerUnsubmitted
Not Done

I don't think the fcntl module is available for Windows. @ikostia @dsp

There are portable locking libraries, but not sure if that is overkill

https://github.com/WoLpH/portalocker

akushner: I don't think the fcntl module is available for Windows. @ikostia @dsp There are portable…
ikostiaUnsubmitted
Not Done

Not sure what you mean by 'overkill'. We certainly need a reliable locking strategy on Windows, as we've seen prefetch locks not being cleaned up several times.
Also, thanks for subscribing me.

ikostia: Not sure what you mean by 'overkill'. We certainly need a reliable locking strategy on Windows…
import os import os
import subprocess import subprocess
from mercurial.i18n import _
from mercurial import ( from mercurial import (
error,
pycompat, pycompat,
) )
if pycompat.iswindows: if pycompat.iswindows:
# no fork on Windows, but we can create a detached process # no fork on Windows, but we can create a detached process
# https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx # https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863.aspx
# No stdlib constant exists for this value # No stdlib constant exists for this value
DETACHED_PROCESS = 0x00000008 DETACHED_PROCESS = 0x00000008
newbases = [oldbase newbases = [oldbase
for oldbase in subcls.__bases__ for oldbase in subcls.__bases__
if oldbase != oldcls] if oldbase != oldcls]
newbases.append(cls) newbases.append(cls)
subcls.__bases__ = tuple(newbases) subcls.__bases__ = tuple(newbases)
setattr(container, classname, cls) setattr(container, classname, cls)
return cls return cls
return wrap return wrap
class fcntllock(object):
"""A fcntllock based lock object. Currently it is always non-blocking.
quarkUnsubmitted
Not Done

I'd rename it to "fcntl lock" in both the class name and comments, since that's different from flock().

See http://chris.improbable.org/2010/12/16/everything-you-never-wanted-to-know-about-file-locking/ for some of their differences.

One of the main differences is fcntl lock will lose after fork while flock() won't. Another seems to be flock() works if the file is opened without write permission.

quark: I'd rename it to "fcntl lock" in both the class name and comments, since that's different from…
Note that since it is fcntllock based, you can accidentally take it multiple
times within one process and the first one to be released will release all
of them. So the caller needs to be careful to not create more than one
instance per lock.
"""
def __init__(self, opener, name, description):
self.path = opener.join(name)
self.description = description
self.fp = None
quarkUnsubmitted
Not Done

nit: Maybe rename fd to fp since it's a file object, not a file descriptor (int). That is more consistent with D642 (as a response to Yuya's comment on a previous patch).

quark: nit: Maybe rename `fd` to `fp` since it's a `file` object, not a file descriptor (int). That is…
def __enter__(self):
path = self.path
if self.fp is not None:
raise error.Abort(_("unable to re-enter lock '%s'") % path)
try:
self.fp = open(path, 'w+')
except (IOError, OSError) as ex:
raise error.Abort(_("unable to create lock file '%s': %s") %
(path, str(ex)))
try:
fcntl.lockf(self.fp, fcntl.LOCK_EX | fcntl.LOCK_NB)
except IOError:
self.fp.close()
self.fp = None
raise error.LockHeld(errno.EAGAIN, path, self.description, '')
return self
def __exit__(self, type, value, traceback):
fp = self.fp
if fp is not None:
fcntl.lockf(fp, fcntl.LOCK_UN)
fp.close()
self.fp = None

remotefilelog/debugcommands.py

# debugcommands.py - debug logic for remotefilelog # debugcommands.py - debug logic for remotefilelog
# #
# Copyright 2013 Facebook, Inc. # Copyright 2013 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
from mercurial import error, filelog, revlog from mercurial import error, filelog, revlog
from mercurial.node import bin, hex, nullid, short from mercurial.node import bin, hex, nullid, short
from mercurial.i18n import _ from mercurial.i18n import _
from hgext3rd import extutil
from . import ( from . import (
constants, constants,
datapack, datapack,
fileserverclient, fileserverclient,
historypack, historypack,
shallowrepo, shallowrepo,
shallowutil, shallowutil,
) )
from .lz4wrapper import lz4decompress from .lz4wrapper import lz4decompress
import hashlib, os import hashlib, os, time
def debugremotefilelog(ui, path, **opts): def debugremotefilelog(ui, path, **opts):
decompress = opts.get('decompress') decompress = opts.get('decompress')
size, firstnode, mapping = parsefileblob(path, decompress) size, firstnode, mapping = parsefileblob(path, decompress)
ui.status(_("size: %s bytes\n") % (size)) ui.status(_("size: %s bytes\n") % (size))
ui.status(_("path: %s \n") % (path)) ui.status(_("path: %s \n") % (path))
"P2 Node".ljust(14), "P2 Node".ljust(14),
"Link Node".ljust(14), "Link Node".ljust(14),
"Copy From")) "Copy From"))
lastfilename = filename lastfilename = filename
ui.write("%s %s %s %s %s\n" % (short(node), short(p1node), ui.write("%s %s %s %s %s\n" % (short(node), short(p1node),
short(p2node), short(linknode), copyfrom)) short(p2node), short(linknode), copyfrom))
def debugwaitonrepack(repo): def debugwaitonrepack(repo):
with repo._lock(repo.svfs, "repacklock", True, None, while True:
None, _('repacking %s') % repo.origroot): try:
pass with extutil.fcntllock(repo.svfs, 'repacklock', ''):
return
except error.LockHeld:
time.sleep(0.1)
def debugwaitonprefetch(repo): def debugwaitonprefetch(repo):
with repo._lock(repo.svfs, "prefetchlock", True, None, with repo._lock(repo.svfs, "prefetchlock", True, None,
None, _('prefetching in %s') % repo.origroot): None, _('prefetching in %s') % repo.origroot):
pass pass

remotefilelog/repack.py

from __future__ import absolute_import from __future__ import absolute_import
import os import os
from hgext3rd.extutil import runshellcommand from hgext3rd.extutil import runshellcommand, fcntllock
from mercurial import ( from mercurial import (
error, error,
extensions, extensions,
mdiff, mdiff,
policy, policy,
scmutil, scmutil,
util, util,
) )
raise ValueError("Function 'isold' is not properly specified") raise ValueError("Function 'isold' is not properly specified")
# use (filename, node) tuple as a keepset key # use (filename, node) tuple as a keepset key
self.keepkeys = keepset(repo, lambda f, n : (f, n)) self.keepkeys = keepset(repo, lambda f, n : (f, n))
self.isold = isold self.isold = isold
def run(self, targetdata, targethistory): def run(self, targetdata, targethistory):
ledger = repackledger() ledger = repackledger()
with self.repo._lock(self.repo.svfs, "repacklock", False, None, with fcntllock(self.repo.svfs, "repacklock",
None, _('repacking %s') % self.repo.origroot): _('repacking %s') % self.repo.origroot):
self.repo.hook('prerepack') self.repo.hook('prerepack')
# Populate ledger from source # Populate ledger from source
self.data.markledger(ledger, options=self.options) self.data.markledger(ledger, options=self.options)
self.history.markledger(ledger, options=self.options) self.history.markledger(ledger, options=self.options)
# Run repack # Run repack
self.repackdata(ledger, targetdata) self.repackdata(ledger, targetdata)

tests/test-extutil.py

# Copyright 2004-present Facebook. All Rights Reserved. # Copyright 2004-present Facebook. All Rights Reserved.
import errno import errno
import os import os
import sys import sys
import time import time
import unittest import unittest
import silenttestrunner import silenttestrunner
from mercurial import (
error,
vfs,
worker,
)
if __name__ == '__main__': if __name__ == '__main__':
sys.path.insert(0, os.path.join(os.environ["TESTDIR"], "..", "hgext3rd")) sys.path.insert(0, os.path.join(os.environ["TESTDIR"], "..", "hgext3rd"))
import extutil import extutil
locktimeout = 25
locksuccess = 24
class ExtutilTests(unittest.TestCase): class ExtutilTests(unittest.TestCase):
def testbgcommandnoblock(self): def testbgcommandnoblock(self):
'''runbgcommand() should return without waiting for the process to '''runbgcommand() should return without waiting for the process to
finish.''' finish.'''
env = os.environ.copy() env = os.environ.copy()
start = time.time() start = time.time()
extutil.runbgcommand(['sleep', '5'], env) extutil.runbgcommand(['sleep', '5'], env)
end = time.time() end = time.time()
'''runbgcommand() should throw if executing the process fails.''' '''runbgcommand() should throw if executing the process fails.'''
env = os.environ.copy() env = os.environ.copy()
try: try:
extutil.runbgcommand([os.devnull, 'arg1', 'arg2'], env) extutil.runbgcommand([os.devnull, 'arg1', 'arg2'], env)
self.fail('expected runbgcommand to fail with EACCES') self.fail('expected runbgcommand to fail with EACCES')
except OSError as ex: except OSError as ex:
self.assertEqual(ex.errno, errno.EACCES) self.assertEqual(ex.errno, errno.EACCES)
def testfcntllock(self):
testtmp = os.environ["TESTTMP"]
opener = vfs.vfs(testtmp)
name = 'testlock'
with extutil.fcntllock(opener, name, 'testing a lock'):
otherlock = self.otherprocesslock(opener, name)
self.assertEquals(otherlock, locktimeout,
"other process should not have taken the lock")
otherlock = self.otherprocesslock(opener, name)
self.assertEquals(otherlock, locksuccess,
"other process should have taken the lock")
def otherprocesslock(self, opener, name):
pid = os.fork()
if pid == 0:
try:
with extutil.fcntllock(opener, name, 'other process lock'):
os._exit(locksuccess)
except error.LockHeld:
os._exit(locktimeout)
else:
p, st = os.waitpid(pid, 0)
st = worker._exitstatus(st) # Convert back to an int
return st
if __name__ == '__main__': if __name__ == '__main__':
silenttestrunner.main(__name__) silenttestrunner.main(__name__)