This is an archive of the discontinued Mercurial Phabricator instance.

hiddenoverride: avoid race condition updating the state file
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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.

This revision was automatically updated to reflect the committed changes.