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

fix: prevent optimize of try_lock in park.rs leading to race conditions (#6355) #6356

Closed
wants to merge 1 commit into from

Conversation

leso-kn
Copy link

@leso-kn leso-kn commented Feb 17, 2024

Motivation

This PR addresses the issue described in #6355, it fixes a segmentation fault in park_timeout() on riscv64.

Solution

Following the rust documentation on black_box in std::hint it's possible to instruct the compiler to prevent optimizing a specific call.

if let Some(mut driver) = self.inner.shared.driver.try_lock() {

Wrapping try_lock() with black_box() in park.rs:74 causes the compiler to block optimizations for the call and prevents omission of the call.

Tested on Alpine Linux edge (riscv64)

See https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/50419 for details.

@github-actions github-actions bot added the R-loom-multi-thread Run loom multi-thread tests on this PR label Feb 17, 2024
@leso-kn leso-kn force-pushed the fix-segfault-park_timeout branch from d308180 to 277802d Compare February 17, 2024 12:27
@leso-kn
Copy link
Author

leso-kn commented Feb 17, 2024

Note on the minrust CI job: The black_box() hint has been stabilized since rust version 1.66.0 (minrust uses 1.63.0)

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Feb 17, 2024
@Darksonn
Copy link
Contributor

Closing for the reasons outlined in #6355 (comment).

@Darksonn Darksonn closed this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime R-loom-multi-thread Run loom multi-thread tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants