Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry opening proc-macro DLLs a few times on Windows. #107595

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Feb 2, 2023

On Windows, the compiler sometimes fails with the message error: LoadLibraryExW failed when trying to load a proc-macro crate. The error seems to occur intermittently, similar to #86929, however, it seems to be almost impossible to reproduce outside of CI environments and thus very hard to debug. The fact that the error only occurs intermittently makes me think that this is a timing related issue.

This PR is an attempt to mitigate the issue by letting the compiler retry a few times when encountering this specific error (which resolved the issue described in #86929).

@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 2, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 2, 2023

📌 Commit 227b285 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2023
@kennykerr
Copy link
Contributor

What does GetLastError return when LoadLibraryExW fails?

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2023
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#106887 (Make const/fn return params more suggestable)
 - rust-lang#107519 (Add type alias for raw OS errors)
 - rust-lang#107551 ( Replace `ConstFnMutClosure` with const closures )
 - rust-lang#107595 (Retry opening proc-macro DLLs a few times on Windows.)
 - rust-lang#107615 (Replace nbsp in all rustdoc code blocks)
 - rust-lang#107621 (Intern external constraints in new solver)
 - rust-lang#107631 (loudly tell people when they change `Cargo.lock`)
 - rust-lang#107632 (Clarifying that .map() returns None if None.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1594b58 into rust-lang:master Feb 4, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 4, 2023
@michaelwoerister michaelwoerister deleted the retry_proc_macro_loading branch February 6, 2023 08:41
@michaelwoerister
Copy link
Member Author

@kennykerr and I discussed this a bit. The underlying OS error is ERROR_NOACCESS which can have many causes. I also doubled-checked that Cargo does not try to pipeline proc-macro crates with other crates consuming them, so if there is a race between the linker still accessing the DLL and a downstream rustc process trying to load it, it would be due to some file system oddity and not because Cargo doesn't synchronize this properly. It's unlikely that a file system on a local computer would exhibit such behavior, but I could imagine that the situation is different in an CI environment where file systems are mounted via network or something like that.

I'll continue to investigate, trying find out what exactly is underneath the ERROR_NOACCESS.

@rodrigorc
Copy link

I've seen this behavior sometimes elsewhere with Windows systems. While I never found out why, I always suspected the antivirus (or Windows Defender or whatever there is), that sees a. EXE/DLL appear from thin air and thinks it has to be checked before it is allowed to be used.

@michaelwoerister
Copy link
Member Author

We've been investigating this further. So far it looks like the mitigation implemented in this PR does not really help.

However, we noticed that all failures occurred in a setup where cargo/rustc were running inside of a Windows docker container, using a target directory that is shared between the host and the container. Moving the target directory to a folder that is not visible to the host seems to resolve the problem (though further testing is needed).

So it might be that a virus scanner or something else running on the container host might be the culprit here.

@mati865
Copy link
Contributor

mati865 commented Feb 21, 2023

Moving the target directory to a folder that is not visible to the host seems to resolve the problem (though further testing is needed).

So that directory is not shared? Then this could also be an issue with sharing mechanism which is not uncommon.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 29, 2023
include source error for LoadLibraryExW

In rust-lang#107595, we added retry behavior for LoadLibraryExW on Windows. If it fails we do not print the underlying error that Windows returned. This made rust-lang#110889 a little harder to debug.

In this PR I am adding the source error in the message if it is available.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 29, 2023
include source error for LoadLibraryExW

In rust-lang#107595, we added retry behavior for LoadLibraryExW on Windows. If it fails we do not print the underlying error that Windows returned. This made rust-lang#110889 a little harder to debug.

In this PR I am adding the source error in the message if it is available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants