Fix Build::compile_objects on error and when used in multithreading #779
Fix Build::compile_objects on error and when used in multithreading #779NobodyXu wants to merge 1 commit into
Build::compile_objects on error and when used in multithreading #779Conversation
|
@thomcc Can we have this PR merged before the release #778? I noticed this bug when working on adding multithreading to |
|
This is complex enough that I'd like to have a bit more time to review. I just got some vaccines this weekend and am slightly under the weather, and would rather not r+ some unsafe code I haven't had time to think about (sorry!). I'll review over the week and try to cut a release next weekend if it seems good. |
|
Thanks @thomcc |
I've submit the PR to ring that will trigger the bug briansmith/ring#1591 |
NobodyXu
changed the title
Build::compile_objects when used in multithreading Build::compile_objects on error and when used in multithreading
|
Initial reaction: I don't think we should be using a static mut for this. Is there some structure by which we could make this a field of the relevant object instead? |
This comment was marked as off-topic.
This comment was marked as off-topic.
I've fixed this by having an immutable global variable for token: static TOKEN: Mutex<Option<jobserver::Acquired>> = Mutex::new(None);the |
|
I've updated the PR to only obtain jobserver token when actually compiling objects, so there is no implicit global token now. Also, I've replaced use of |
| match client.available() { | ||
| Ok(0) | Err(_) => { | ||
| // There is no available tokens. | ||
| // This is probably because `cc::Build::compile_objects` is called in | ||
| // `build.rs` where the `cargo` holds the token. | ||
| // | ||
| // In that case, we will release one raw token just to prevent deadlock, | ||
| // but this will increase the allowed global concurrency by one. | ||
| let _ = client.release_raw(); | ||
| } |
There was a problem hiding this comment.
@joshtriplett I implemented what you requested (only acquire and release token when actually compiling) but with a catch.
cargo would acquire one token before running the build.rs, so the available token could be 0 when we obtained the jobserver via jobserver::Client::from_env and causing deadlock.
To prevent this, I added this piece of code which release one token into the jobserver, but it cannot be re-acquired unless I added back the global state for the obtained token.
There was a problem hiding this comment.
Oops, the CI is still in deadlock.
Before this commit, each call to `Build::compile_objects` release a raw token regardless of whether there is a token acquired by this process. As such, if `Build::compile_objects` is used in multithreading context, then it would call `Client::release_raw` for each thread and thus increasing the parallelism far beyond the limit. This commit fixed the bug by not acquiring the implicit global token when initializing global `jobserver::Client` and only obtain token when compiling objects. It also replaced use of `Option<jobserver::Client>` with `MaybeUninit<jobserver::Client>` and eliminate an `Option::unwrap()` inside fn `jobserver`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
|
Hmmm given the deadlock caused by PR and the global state required to fix it, I suppose that it's better leave it as-is for now. |
3 participants
Before this commit, each call to
Build::compile_objectsrelease a rawtoken regardless of whether there is a token acquired by this process.
As such, if
Build::compile_objectsis used in multithreading context,then it would call
Client::release_rawfor each thread and thusincreasing the parallelism far beyond the limit.
This commit fixed the bug by not acquiring the implicit global token
when initializing global
jobserver::Clientand only obtain token whencompiling objects.
Signed-off-by: Jiahao XU Jiahao_XU@outlook.com