Replace fcntl-based file lock with flock

[nbdd0121]

WSL1 does not support fcntl-based lock and will always report success,
therefore creating a race condition when multiple rustc processes are
modifying shared data such as search-index.js. WSL1 does however
support flock.

flock is supported by all unix-like platforms. The only caveat is that
Linux 2.6.11 or earlier does not support flock on NFS mounts, but as
the minimum supported Linux version is 2.6.18, it is not an issue.

Fixes #72157

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nbdd0121]

r? @cuviper

[cuviper]

flock is supported by all unix-like platforms.

I think this is overstated -- it's a syscall of BSD origin, and my Linux man-page says it appears on most UNIX systems, but in particular it seems Solaris still does not have it.

• no flock here: https://docs.oracle.com/cd/E88353_01/html/E37841/makehtml-id-6.html
• noted for portability here: https://www.perkin.org.uk/posts/solaris-portability-flock.html

Since there's nothing really wrong with the current fcntl approach, and it's POSIX functionality, I suggest we keep using that on most unix targets. We can change to flock for linux alone, and specifically comment that we're doing this for WSL1's sake.

(I know I suggested switching all unix in #72157, but now I know it is a portability hazard.)

[cuviper]

I found that Cargo uses flock, but it has a caution about blocking on NFS:

https://github.com/rust-lang/cargo/blob/13bded9a335a855a15b95f54779d7f8a889404dd/src/cargo/util/flock.rs#L284-L291

    // File locking on Unix is currently implemented via `flock`, which is known
    // to be broken on NFS. We could in theory just ignore errors that happen on
    // NFS, but apparently the failure mode [1] for `flock` on NFS is **blocking
    // forever**, even if the "non-blocking" flag is passed!
    //
    // As a result, we just skip all file locks entirely on NFS mounts. That
    // should avoid calling any `flock` functions at all, and it wouldn't work
    // there anyway.
    //
    // [1]: https://github.com/rust-lang/cargo/issues/2615

They use this for all unix, except on Solaris they just ignore locking:

https://github.com/rust-lang/cargo/blob/13bded9a335a855a15b95f54779d7f8a889404dd/src/cargo/util/flock.rs#L284-L291

    #[cfg(target_os = "solaris")]
    fn flock(file: &File, flag: libc::c_int) -> Result<()> {
        // Solaris lacks flock(), so simply succeed with a no-op
        Ok(())
    }

cc @pfmooney who rewrote that recently.

[cuviper]

cc @alexcrichton too, who reviewed that cargo change.

[pfmooney]

cc @pfmooney who rewrote that recently.

Much of that locking code was lifted out of fs2, which had become unsuitable due to maintenance concerns (not accepting patches to support new systems/targets). Porting over the fcntl()-based locking for Solaris targets was considered, but was skipped for the sake of simplicity since concurrent (and conflicting) cargo operations were identified as infrequent and not a high priority.

A flock() implementation was prioritized in illumos (drawing its lineage back to OpenSolaris) due to the differing semantics between flock() and fcntl()-based locks, specifically around fork() behavior. I'm not sure how much those differences would relate to your change here.

[nbdd0121]

flock is supported by all unix-like platforms.

I think this is overstated -- it's a syscall of BSD origin, and my Linux man-page says it appears on most UNIX systems, but in particular it seems Solaris still does not have it.

Ah indeed. So here is what I originally do to ensure it's okay:

• Linux man page says its available on most UNIX systems as you pointed out.
• I checked libc, and flock is unconditionally available on all unix platforms https://github.com/rust-lang/libc/blob/0.2.70/src/unix/mod.rs#L1062.
• I checked all UNIX systems where rustc is currently supported https://forge.rust-lang.org/release/platform-support.html, to make sure it indeed have flock.

I should probably framed it as "all unix-like platforms" supported by rustc.

• no flock here: https://docs.oracle.com/cd/E88353_01/html/E37841/makehtml-id-6.html
• noted for portability here: https://www.perkin.org.uk/posts/solaris-portability-flock.html

Solaris isn't supported by rustc currently. I am not sure if that is a convincing enough reason to not keep the fcntl version?

I found that Cargo uses flock, but it has a caution about blocking on NFS:

https://github.com/rust-lang/cargo/blob/13bded9a335a855a15b95f54779d7f8a889404dd/src/cargo/util/flock.rs#L284-L291

    // File locking on Unix is currently implemented via `flock`, which is known
    // to be broken on NFS. We could in theory just ignore errors that happen on
    // NFS, but apparently the failure mode [1] for `flock` on NFS is **blocking
    // forever**, even if the "non-blocking" flag is passed!
    //
    // As a result, we just skip all file locks entirely on NFS mounts. That
    // should avoid calling any `flock` functions at all, and it wouldn't work
    // there anyway.
    //
    // [1]: https://github.com/rust-lang/cargo/issues/2615

This is interesting. However Linux man page suggested that on Linux, flock is emulated by fcntl lock on NFS drives (I double checked Linux source and it is true). So the "hang" would happen with the current implementation as well. That particular issue is likely a misconfiguration which enabled lock feature on non-NFSv4 drives.

[nbdd0121]

I tested flock-based implementation on NFSv4 and it works fine.

BTW the current approach used in Cargo does affect people as well by not locking on NFS, see rust-lang/cargo#4629.

[alexcrichton]

FWIW we treat locking as quite optional in Cargo due to it being too restrictive if we require it everywhere. We try to use it everywhere we can but it's empirically had a lot of bugs and had to adjust a lot over time. I doubt Cargo is doing "the best thing" for all platforms under the sun still, and it probably still has issues that need to be worked through.

[nbdd0121]

@cuviper Is Solaris a platform that rustc ever intends to run on? If so I can bring back fcntl version and use it only for Solaris/Illumos. For other Unices I personally would use flock version because its nicer semantics but I'm okay with either method.

[pfmooney]

@cuviper Is Solaris a platform that rustc ever intends to run on? If so I can bring back fcntl version and use it only for Solaris/Illumos. For other Unices I personally would use flock version because its nicer semantics but I'm okay with either method.

flock() is present on illumos, so the fcntl()-based locking is not necessary there. The solaris target is currently being used by illumos systems as they transition over to the somewhat recently created illumos target. (We're still waiting on it to be enabled as a CI build)

[cuviper]

I tested flock-based implementation on NFSv4 and it works fine.

The thing I'm worried about from that comment is the "failure mode" aspect, which I inferred to mean the behavior when the network disappears. But maybe it's also problematic for NFSv3?

@cuviper Is Solaris a platform that rustc ever intends to run on?

I don't know -- it seems the present folks really care about illumos, not solaris proper, which will be getting an independent target as mentioned.

My feeling is that we should be really careful about changing this, given the known hazards. We do need to care about it more than Cargo does itself, because rustc and rustdoc are definitely going to be called in parallel on a regular basis. In the extreme, we'd only change at all for WSL where we know fcntl doesn't work right -- can we tell from uname or something?

[nbdd0121]

The thing I'm worried about from that comment is the "failure mode" aspect, which I inferred to mean the behavior when the network disappears. But maybe it's also problematic for NFSv3?

It's up to the configuration of NFS. If NFS is configured to be uninterruptable and infinite retry, then a network disconnection will cause everything to hang. In that case it does not matter how locks behave, because we will run into issues when doing other IOs as well. Most NFS-related issues are actually poor configuration.

If anything can break flock on Linux it'll be NFS. But Linux essentially emulates flock using fcntl on NFS drives, so if flock breaks, fcntl would break as well. I'm not sure about BSD's handling on this - but I think convert fcntl to flock on Linux is pretty safe.

My feeling is that we should be really careful about changing this, given the known hazards. We do need to care about it more than Cargo does itself, because rustc and rustdoc are definitely going to be called in parallel on a regular basis. In the extreme, we'd only change at all for WSL where we know fcntl doesn't work right -- can we tell from uname or something?

We could check if uname contains Microsoft.

[cuviper]

@bors r+

[bors]

📌 Commit a05acbf has been approved by cuviper