( )⚙ D139 hiddenoverride: avoid race condition updating the state file

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D139

hiddenoverride: avoid race condition updating the state file
ClosedPublic

Authored by quark on Jul 18 2017, 6:28 PM.
Tags
None
Subscribers
kulshrax

Details

Reviewers
kulshrax
Group Reviewers
Restricted Project
Commits
rFBHGX33b39de2acca: hiddenoverride: avoid race condition updating the state file
Summary

Previously there is a race condition:

origpinned = loadpinnednodes(repo)
newpinned = ....
with repo.lock(): # the lock might be taken by process X
    # get the lock after some time. at this time, the state file might
    # be updated by process X but we are not aware of it.
    savepinnednodes(...) # process X's change gets discarded

This patch solves that by making the write function takes a delta instead of
full content, and apply that delta inside a lock. Using repo lock is
expensive so we use a single-file lightweight flock instead.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 18 2017, 6:28 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 18 2017, 6:28 PM
kulshrax accepted this revision.Jul 18 2017, 7:02 PM
kulshrax added a subscriber: kulshrax.

I'm going to accept this since this is blocking our internal release, but there are some portability concerns here since POSIX advisory locking isn't available on Windows, for example. That should probably be addressed in a separate patch. It might be worth making note of this as a comment in the code?

This revision is now accepted and ready to land.Jul 18 2017, 7:02 PM
quark edited the summary of this revision. (Show Details)Jul 18 2017, 7:05 PM
quark updated this revision to Diff 290.
quark added a comment.Jul 18 2017, 7:06 PM
In D139#2040, @kulshrax wrote:

I'm going to accept this since this is blocking our internal release, but there are some portability concerns here since POSIX advisory locking isn't available on Windows, for example. That should probably be addressed in a separate patch. It might be worth making note of this as a comment in the code?

Thanks! I have updated the code so it should fallback to less efficient Mercurial lock gracefully.

Closed by commit rFBHGX33b39de2acca: hiddenoverride: avoid race condition updating the state file (authored by quark). · Explain WhyJul 18 2017, 7:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 290

hgext3rd/fbamend/hiddenoverride.py

# hiddenoverride.py - lightweight hidden-ness override # hiddenoverride.py - lightweight hidden-ness override
# #
# Copyright 2017 Facebook, Inc. # Copyright 2017 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
import contextlib
import os
from mercurial import ( from mercurial import (
dispatch, dispatch,
error, error,
extensions, extensions,
lock as lockmod,
obsolete, obsolete,
repoview, repoview,
util,
vfs as vfsmod,
) )
def uisetup(ui): def uisetup(ui):
extensions.wrapfunction(repoview, 'pinnedrevs', pinnedrevs) extensions.wrapfunction(repoview, 'pinnedrevs', pinnedrevs)
extensions.wrapfunction(dispatch, 'runcommand', runcommand) extensions.wrapfunction(dispatch, 'runcommand', runcommand)
extensions.wrapfunction(obsolete, 'createmarkers', createmarkers) extensions.wrapfunction(obsolete, 'createmarkers', createmarkers)
def pinnedrevs(orig, repo): def pinnedrevs(orig, repo):
while True: while True:
node = content[offset:offset + 20] node = content[offset:offset + 20]
if not node: if not node:
break break
if node in nodemap: if node in nodemap:
yield node yield node
offset += 20 offset += 20
def savepinnednodes(repo, nodes): def shouldpinnodes(repo):
with repo.lock(), repo.svfs.open('obsinhibit', 'wb', atomictemp=True) as f: """get nodes that should be pinned: working parent + bookmarks for now"""
result = set()
if repo and repo.local():
# working copy parent
try:
wnode = repo.vfs('dirstate').read(20)
result.add(wnode)
except Exception:
pass
# bookmarks
result.update(repo._bookmarks.values())
return result
@contextlib.contextmanager
def flock(lockpath):
# best effort lightweight lock
try:
import fcntl
fcntl.flock
except ImportError:
# fallback to Mercurial lock
vfs = vfsmod.vfs(os.path.dirname(lockpath))
with lockmod.lock(vfs, os.path.basename(lockpath)):
yield
return
# make sure lock file exists
util.makedirs(os.path.dirname(lockpath))
with open(lockpath, 'a'):
pass
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)
def savepinnednodes(repo, newpin, newunpin):
# take a narrowed lock so it does not affect repo lock
with flock(repo.svfs.join('obsinhibit.lock')):
nodes = set(loadpinnednodes(repo))
nodes |= set(newpin)
nodes -= set(newunpin)
with util.atomictempfile(repo.svfs.join('obsinhibit')) as f:
f.write(''.join(nodes)) f.write(''.join(nodes))
def runcommand(orig, lui, repo, cmd, fullargs, *args): def runcommand(orig, lui, repo, cmd, fullargs, *args):
shouldpinbefore = shouldpinnodes(repo)
result = orig(lui, repo, cmd, fullargs, *args) result = orig(lui, repo, cmd, fullargs, *args)
# after a command completes, make sure working copy parent and all # after a command completes, make sure working copy parent and all
# bookmarks get "pinned". # bookmarks get "pinned".
if repo and repo.local(): newpin = shouldpinnodes(repo) - shouldpinbefore
unfi = repo.unfiltered() newunpin = getattr(repo, '_tounpinnodes', set())
wnode = None # only do a write if something has changed
try: if newpin or newunpin:
# read dirstate directly to avoid dirstate object overhead savepinnednodes(repo, newpin, newunpin)
wnode = repo.vfs('dirstate').read(20)
except Exception:
pass
tounpin = getattr(unfi, '_tounpinnodes', set())
origpinned = set(loadpinnednodes(repo))
pinned = origpinned - tounpin
if wnode:
pinned.add(wnode)
pinned.update(unfi._bookmarks.values())
if pinned != origpinned:
savepinnednodes(repo, pinned)
return result return result
def createmarkers(orig, repo, rels, *args, **kwargs): def createmarkers(orig, repo, rels, *args, **kwargs):
# this is a way to unpin revs - precursors are unpinned # this is a way to unpin revs - precursors are unpinned
unfi = repo.unfiltered() unfi = repo.unfiltered()
tounpin = getattr(unfi, '_tounpinnodes', set()) tounpin = getattr(unfi, '_tounpinnodes', set())
for r in rels: for r in rels:
try: try:
tounpin.add(r[0].node()) tounpin.add(r[0].node())
except error.RepoLookupError: except error.RepoLookupError:
pass pass
unfi._tounpinnodes = tounpin unfi._tounpinnodes = tounpin
return orig(repo, rels, *args, **kwargs) return orig(repo, rels, *args, **kwargs)

tests/test-fbamend.t

rebasing 4:01dd7a39383a "bar" rebasing 4:01dd7a39383a "bar"
$ hg log -G -T '{rev} {node|short} {desc} {bookmarks}\n' $ hg log -G -T '{rev} {node|short} {desc} {bookmarks}\n'
o 8 9752120dcffe bar o 8 9752120dcffe bar
| |
o 7 bc3b6a46cdb4 commit --amend message o 7 bc3b6a46cdb4 commit --amend message
| |
@ 6 3a4d2824efc1 message from exec @ 6 3a4d2824efc1 message from exec
| |
| x 2 048e86baa19d message from exec
|/
| o 1 3f6197b00eba extra commit to test multiple heads | o 1 3f6197b00eba extra commit to test multiple heads
|/ |/
o 0 cb9a9f314b8b a o 0 cb9a9f314b8b a
$ echo "cc" > cc $ echo "cc" > cc
$ hg add cc $ hg add cc
$ hg amend --rebase --traceback $ hg amend --rebase --traceback
rebasing the children of 1c16dd8e35d2.preamend rebasing the children of 1c16dd8e35d2.preamend