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

LocalPool's ThreadNotify atomic orderings are overly strict #2601

Closed
talchas opened this issue May 21, 2022 · 0 comments · Fixed by #2608
Closed

LocalPool's ThreadNotify atomic orderings are overly strict #2601

talchas opened this issue May 21, 2022 · 0 comments · Fixed by #2608

Comments

@talchas
Copy link

talchas commented May 21, 2022

local_pool.rs:94 is

            let unparked = thread_notify.unparked.swap(false, Ordering::Acquire);
            if !unparked {
                thread::park();
                thread_notify.unparked.store(false, Ordering::Release);
            }

The only other thread that accesses this is the ArcWake impl, which uses Relaxed, which means these orderings cannot synchronize with anything, and thus are useless. SinceWakers can be ignored and just loop { future.poll() } is valid, all three orderings can be Relaxed. If it wasn't then Release in wake_by_ref combined with Acquire in both of these operations would make sense (and possibly a loop instead of a single check, in case of thread::unpark being spammed randomly); it might also be vaguely defensible to do those orderings anyways in order to handle buggy Future impls that don't properly redo wake() if they know it has been called. In no case is there any use to the current orderings and they're just confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant