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

runtime: Remove last slab dependency #2917

Merged
merged 1 commit into from
Nov 5, 2020
Merged

Conversation

bdonlan
Copy link
Contributor

@bdonlan bdonlan commented Oct 6, 2020

This removes the last slab dependency by replacing the current slab-based
JoinHandle tracking with one based on HashMap instead.

Motivation

Cleaning up dependencies

Solution

Slab was only used to track JoinHandles to ensure that all threads are fully cleaned up before returning from shutdown (this, in turn, is intended to avoid valgrind false-positives). Since this path isn't particularly performance-critical, this change replaces this use case with a HashMap instead.

@bdonlan bdonlan requested a review from carllerche October 6, 2020 19:37
@bdonlan
Copy link
Contributor Author

bdonlan commented Oct 6, 2020

CI blocked on tokio-rs/loom#175

@carllerche
Copy link
Member

loom 0.3.6 is out.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 thanks.

@bdonlan bdonlan force-pushed the rm-slab branch 2 times, most recently from 24d92e3 to 0a14d9b Compare October 8, 2020 22:27
@Darksonn Darksonn added A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-blocking Module: tokio/task/blocking labels Oct 9, 2020
@Darksonn
Copy link
Contributor

Darksonn commented Oct 9, 2020

Loom claims to be unhappy about a deadlock in shutdown code.

@bdonlan
Copy link
Contributor Author

bdonlan commented Oct 9, 2020

Hmm?

thread 'runtime::tests::loom_pool::group_b::pool_shutdown' panicked at 'assertion failed: state.notified', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/loom-0.3.6/src/rt/notify.rs:112:13
stack backtrace:
   0: std::panicking::begin_panic
   1: scoped_tls::ScopedKey<T>::with
   2: loom::rt::notify::Notify::wait
   3: loom::thread::JoinHandle<T>::join
   4: <tokio::runtime::blocking::pool::BlockingPool as core::ops::drop::Drop>::drop
   5: core::ptr::drop_in_place
   6: core::ops::function::FnOnce::call_once{{vtable.shim}}
   7: loom::rt::scheduler::spawn_threads::{{closure}}::{{closure}}
   8: generator::stack::StackBox<F>::call_once
   9: generator::gen_impl::gen_init
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread 'runtime::tests::loom_pool::group_b::pool_shutdown' panicked at 'deadlock; threads = [(Id(0), Blocked), (Id(1), Terminated), (Id(2), Terminated), (Id(3), Terminated)]', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/loom-0.3.6/src/rt/execution.rs:207:13

We're deadlocked on JoinHandle.wait() but the blocked thread is the only one alive. There shouldn't be any JoinHandles for the initial thread, so it can't wait on itself. Is this a bug in loom? @carllerche

@bdonlan
Copy link
Contributor Author

bdonlan commented Oct 9, 2020

I'm unable to reproduce this failure locally. Since a rebase is needed anyway, rebasing and seeing if it fails again.

@bdonlan
Copy link
Contributor Author

bdonlan commented Oct 9, 2020

As far as I can tell, this seems like it might be a loom bug - filed tokio-rs/loom#177

@bdonlan
Copy link
Contributor Author

bdonlan commented Oct 22, 2020

Found the loom bug - we're waiting on joinhandles in indeterminate order. Fixing.

This removes the last slab dependency by replacing the current slab-based
JoinHandle tracking with one based on HashMap instead.
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@carllerche carllerche merged commit d7e3fcb into tokio-rs:master Nov 5, 2020
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 C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-blocking Module: tokio/task/blocking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants