rustc_trans: atomically write .rmeta outputs to avoid races.

[eddyb]

Fixes #45841 in a similar vein to how LLVM writes archives: write a temporary file and then rename it.

r? @alexcrichton

[alexcrichton]

Thanks for tracking this down!

I'm not sure I understand what's going on here in terms of where the race is coming from though. In general I don't really trust our ability to do anything atomic on the filesystem in a cross-platform and cross-filesystem manner, so I think the best solution may be to fix this elsewhere.

That being said though I also don't understand the race here, so it'd be helpful if you could explain in more detail what the race is? I'm curious why we've never seen this before because if it's just a bog-standard race it seems like we should be seeing this all over the ecosystem, so I'm not sure how #45841 is different from other setups.

[eddyb]

They're outputting .rmeta, for some reason to do with cargo check, and other rustc invocations are searching the same output directory for their own dependencies and see the partial .rmeta file.
AFAICT the only reason we don't have this problem with .rlib is because LLVM fixes it for us.

With enough parallelism, I suspect cargo check might be able to reproduce this, unless the rustc scanning the output directory is doing so because it wasn't invoked with --extern's, by cargo.

[alexcrichton]

Hm ok, that doesn't really make me feel good about "why isn't anyone else seeing this" but presumably cargo check isn't used that often I guess? I also diagree that "LLVM fixes it for us" is true in the sense that the loader just ignores all errors when loading rlibs and otherwise is pretty defensive about ensuring we're only looking at valid rlibs. Can we do the same when loading metadata these rmeta files? Instead of trying to write them atomically just instead ignore files if they look erroneous?

[cramertj]

@alexcrichton Would it be possible for us to land this fix on a provisional basis so that we can confirm this fixes the Fuchsia build? If it is not the correct fix, we need to continue debugging the issue and it'd be good to know that as soon as possible.

Once it's merged (or beforehand) we can open an issue tracking possible improvements.

[alexcrichton]

@cramertj I haven't heard a response to my previous comment and AFAIK it won't be too hard to implement. That coupled with the queue generally always moving at a snail's pace I don't think will delay this much.

[arielb1]

@alexcrichton

I don't think there's a stable-across-filesystems semantics for the state of files while they are being written to.

For example, if a machine crashes while rustc is running, some filesystems leave behind files that contain various kinds of garbage (if you're lucky, the garbage is guaranteed not to contain some other user's passwords).

Therefore, I'll rather use the rename interface, which is guaranteed not to leave garbage in various annoying states around (well, either that or have a checksum/checksum Merkle tree of the file contents which we verify upon loading, but that would be too slow).

[alexcrichton]

Ok we can go with this for now and see how far we get.

@bors: r+

To be clear I'm worried about bugs like rust-lang/cargo#800 where rename isn't supported in all situations in all filesystems. I realize that specific issue is not exactly applicable to the compiler here but it's an example of how "things don't always work as documented" so putting a little effort into not relying on filesystems/posix/etc can help you go a long way. We've got a huge number of workarounds in Cargo for various bugs here and there with things like virtualbox directories, nfs directories, etc. I certainly hope that rename here works with a temporary directory in the same output directory but I would not be certain it would work.

I realize though that this line of reasoning never really resonates with anyone and it sounds like there's not many volunteers here to put in some extra legwork.

[bors]

📌 Commit f595280 has been approved by alexcrichton

[bors]

⌛ Testing commit f595280 with merge 3759880e7e86ab02bc1813edc61aa29b68cfcac5...

[bors]

💔 Test failed - status-appveyor

[eddyb]

@bors retry

• appveyor seems to be slowed down today

[bors]

⌛ Testing commit f595280 with merge 8752aee...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8752aee to master...