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

Guard run_executor of local_pool.rs against use of park / unpark in user-code. #2010

Merged
merged 2 commits into from
Dec 26, 2019

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Dec 19, 2019

If user code that is run as a result of polling the futures in the run_executor loop of local_pool.rs makes use of the park / unpark APIs, execution may stall due to wakeups getting "lost". This is prevented in this PR through an additional AtomicBool that is in full control of the code in local_pool.rs.

This problem appeared in practice in the context of using the functions from local_pool.rs in combination with async_std::net::stream::TcpStream::connect(), which uses spawn_blocking which accesses a thread pool behind a once_cell::sync::Lazy, which in turn parks/unparks threads on concurrent access to the lazy value while initialisation is ongoing. As a result it sporadically happened that execution would stall as a "token" made available by unpark() would be "consumed" by once_cell and execution stall on the next park(). The included test reconstructs such a scenario.

Though not intentional, this change also seems to have a small positive impact on the benchmarks, possibly due to avoiding unnecessary calls to park / unpark:

 name                                  before ns/iter  after ns/iter  diff ns/iter   diff %  speedup 
 thread_yield_multi_thread             6,418,990       6,322,004           -96,986   -1.51%   x 1.02 
 thread_yield_single_thread_many_wait  310,494         232,616             -77,878  -25.08%   x 1.33 
 thread_yield_single_thread_one_wait   235,630         160,431             -75,199  -31.91%   x 1.47 

Lastly, a possible alternative may be to use e.g. crossbeam::sync::Parker though that would introduce a new dependency.

If user code that is run as a result of polling the futures
in the `run_executor` loop of `local_pool.rs` makes use of
the park / unpark APIs, execution may stall due to wakeups
getting "lost". This is prevented (and thereby unnecessary
calls to park / unpark avoided) through an additional
`AtomicBool` that is in full control of the code in
`local_pool.rs`.
@romanb
Copy link
Contributor Author

romanb commented Dec 19, 2019

cc @twittner who also looked into this issue.

@cramertj cramertj merged commit 34bca9d into rust-lang:master Dec 26, 2019
@cramertj
Copy link
Member

Thanks!

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 this pull request may close these issues.

2 participants