This is an archive of the discontinued Mercurial Phabricator instance.

repack: move to flock based locking

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


Group Reviewers
Restricted Project
rFBHGX1c05727c4087: repack: move to fcntllock based locking

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

rFBHGX Facebook Mercurial Extensions
Automatic diff as part of commit; lint not applicable.
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.


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

See 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.


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 inline comments.

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

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. could also be a reasonable cross-platform choice.

@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

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

This revision was automatically updated to reflect the committed changes.
ikostia added inline comments.Nov 30 2017, 6:31 AM

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.