Fix #2641, #2535, and `shutdown_timeout` blocking for no reason #2649

emgre · 2020-07-08T14:36:50Z

Issue Hello world examples non-deterministrically leak under multi-threaded runtime #2641 was caused by the fact that the thread pool didn't kept the JoinHandle of its threads. Now, they are kept in a vector and properly joined at shutdown. If the thread does not return within the timeout specified, the thread is not joined and left to cleanup by the OS.
- ⚠ The tests rt_common threaded_scheduler_1_thread::runtime_in_thread_local threaded_scheduler_4_threads::runtime_in_thread_local now blocks indefinitely on Windows for some unknown reason. Upon investigation, it seems like the thread properly returns so my only guess is that there is a deadlock in one of the destructor of variables in the thread local storage. It would explain why it only affects Windows, since on unix systems, the destructors of TLS variables do not seem to get called.
Issue Dropping threaded runtime with time and IO enabled results in memory leaks #2535 was caused by a cyclic Arc that prevented the IO driver and the worker threads from being freed.
- ⚠ My solution was the easiest way to fix it that I could find, but a second look from someone with more knowledge of the codebase is needed. There are a lot of RefCell of Arc in that part of tokio, and it worries me.
There was another issue I found while doing these changes, but I don't think it's in the issue tracker. The shutdown_timeout would unnecessarily block and leave the threads running when no job was dispatched to the queue. It only needed to notify the threads to wake them up. When dropping the runtime, the notification is sent by Drop of ThreadPool so it properly shutdowns.
~~⚠ The test tcp_into_split drop_write blocks indefinitely on Windows for unknown reasons. However, this was not introduced by my changes, I have the same behaviour on the master branch.~~ This was fixed in chore: fix windows CI #2597.

I divided my changes in three commits for easier reading.

JoinHandle of threads created by the pool are now tracked and properly joined at shutdown. If the thread does not return within the timeout, then it's not joined and left to the OS for cleanup.

In threaded runtime, the unparker now owns a weak reference to the inner data. This breaks the cycle of Arc and properly releases the io driver and its worker threads.

emgre · 2020-07-08T15:22:10Z

Oh well, I just found through the CI that loom doesn't have Weak... Also, some tasks failed because crates.io returned 504.

Darksonn

I can't review all parts of this PR, but I have two comments:

tokio/src/runtime/blocking/pool.rs

tokio/tests/tcp_into_split.rs

Darksonn · 2020-07-20T08:28:22Z

As I understand it, this PR is blocked on loom getting a Weak type?

emgre · 2020-07-20T12:33:48Z

@Darksonn Indeed. Also, there's still one test that blocks on Windows that I wasn't able to fix (rt_common threaded_scheduler_1_thread::runtime_in_thread_local)

hawkw

Overall, this looks correct to me, although adding a mock Weak to loom is, of course, a blocker.

I had a couple minor suggestions.

tokio/tests/tcp_into_split.rs

tokio/src/runtime/blocking/shutdown.rs

tokio/src/runtime/blocking/pool.rs

carllerche · 2020-07-20T21:19:14Z

Hi! Thanks for this work. I apologize for the delay in responding. I've been heads down in a big chunk of work.

Regarding the time driver cycle bug. I would prefer avoiding the weak ref as it would add a bunch of upgrades in a hot path. Instead, we can add a shutdown(&mut self) fn to Park (here). For the time driver, this would be implemented by dropping all entries in the ring, which, in turn will drop the wakers. This should break the cycle.

jadamcrain · 2020-07-21T05:18:47Z

@emgre Can you look into @carllerche's suggestion? If we don't need weak ref, then loom isn't an issue and we're unblocked.

…sue-2641

…sue-2641 # Conflicts: # tokio/src/runtime/blocking/shutdown.rs

emgre · 2020-07-23T20:01:15Z

I implemented what @carllerche suggested and it fixes the leak issues we have in our codebase. However, I'm still having trouble with loom. I think it's missing a unsafe impl<T> Send for JoinHandle<T> {}, but I was not able to reproduce what the CI does. Running RUSTFLAGS="--cfg loom --cfg tokio_unstable" cargo test --lib --release --features "full" -- --nocapture loom_pool::group_a on my machine successfully compiles...

cc @jadamcrain

hawkw · 2020-07-23T21:45:50Z

I implemented what @carllerche suggested and it fixes the leak issues we have in our codebase. However, I'm still having trouble with loom. I think it's missing a unsafe impl<T> Send for JoinHandle<T> {}, but I was not able to reproduce what the CI does. Running RUSTFLAGS="--cfg loom --cfg tokio_unstable" cargo test --lib --release --features "full" -- --nocapture loom_pool::group_a on my machine successfully compiles...

cc @jadamcrain

@emgre Ah, it looks like there was a loom PR to add missing Send + Sync impls for the loom version of JoinHandle (tokio-rs/loom#145) that has merged but hasn't been released yet. I'll put together a new loom release, which should hopefully fix that and we can get this PR merged!

It's a little weird that it does compile for you locally --- you wouldn't happen to have a git dependency on loom or something, would you?

emgre · 2020-07-23T23:17:31Z

@hawkw I had loom checked out locally on the master branch when I tried it, so it had the changes you pointed to 🤦‍♂️

Darksonn · 2020-07-27T17:23:48Z

I think the new version of loom is out now.

…sue-2641

emgre · 2020-07-27T18:43:13Z

The CI is now green ✔

The only remaining thing to fix is this test that currently deadlocks on Windows:

tokio/tokio/tests/rt_common.rs

Line 873 in d5507b3

fn runtime_in_thread_local() {

I worked on it this morning without success 😓

emgre · 2020-07-28T13:23:26Z

I was able to reproduce the issue with the thread-local storage destructor not returning from a thread::join() and I'm now convinced that it's a bug in libstd, not tokio. I opened this issue: rust-lang/rust#74875

carllerche

Looking good 👍 thanks for sticking with this. I added some comments, but I think this is getting close.

tokio/tests/rt_common.rs

tokio/src/runtime/blocking/pool.rs

carllerche · 2020-07-28T20:31:52Z

To fix formatting, run:

rustfmt --check --edition 2018 $(find . -name '*.rs' -print)

carllerche

Thanks! One last final tweak and I think we should be good to go 👍

tokio/src/runtime/blocking/pool.rs

carllerche

Thanks for sticking with it 👍

Summary: Updating to a newer version of Tokio breaks hgcli. This seems to be happening because Tokio fixed a bug where shutting down your runtime could leak threads (tokio-rs/tokio#2649), which results in the runtime refusing to shutdown if you have a thread waiting to read on stdin (note: I didn't try to back that out specifically to confirm). Unfortunately, we have that in hgcli, because we usually kick off a stdin read before we know if we'll need the data or not. Here's the backtrace for a blocked shutdown P163996251. We were already trying to shut down the runtime and ignore every outstanding work (because we ensure earlier that there is nothing outstanding), so let's just try even harder. Reviewed By: StanislavGlebik Differential Revision: D25952430 fbshipit-source-id: ae3a1413790cf81a5d990220f2dc5d4599219f73

emgre added 4 commits July 8, 2020 10:17

Fix tokio-rs#2641

f06c23d

JoinHandle of threads created by the pool are now tracked and properly joined at shutdown. If the thread does not return within the timeout, then it's not joined and left to the OS for cleanup.

Fix Runtime::shutdown_timeout blocking even if no task is executed.

0321706

Fix tokio-rs#2535.

1676e6e

In threaded runtime, the unparker now owns a weak reference to the inner data. This breaks the cycle of Arc and properly releases the io driver and its worker threads.

Pass CI.

9e181e5

This was referenced Jul 8, 2020

Investigate threaded runtime with single thread shutdown on Windows #2499

Open

Missing std::sync::Weak tokio-rs/loom#156

Open

Tokio leaks file descriptors #2635

Closed

Restart CI.

70e0840

Darksonn reviewed Jul 9, 2020

View reviewed changes

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

tokio/tests/tcp_into_split.rs Outdated Show resolved Hide resolved

Remove .ok() in blocking/pool.rs.

451c148

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime labels Jul 20, 2020

Darksonn added the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Jul 20, 2020

hawkw reviewed Jul 20, 2020

View reviewed changes

tokio/tests/tcp_into_split.rs Outdated Show resolved Hide resolved

tokio/src/runtime/blocking/shutdown.rs Outdated Show resolved Hide resolved

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

emgre added 7 commits July 21, 2020 09:53

Merge remote-tracking branch 'remotes/upstream/master' into bugfix-is…

a92f2da

…sue-2641

PR comments

0890836

Fix tokio-rs#2535 with a shutdown method instead of weak pointers.

a018587

Merge remote-tracking branch 'remotes/upstream/master' into bugfix-is…

59074d2

…sue-2641 # Conflicts: # tokio/src/runtime/blocking/shutdown.rs

Revert root Cargo.toml.

f09f748

Added checks to atomic_stack.

17490d8

Fix SIGSEGV in atomic_stack.

f1b3090

emgre force-pushed the bugfix-issue-2641 branch from 938ae13 to f1b3090 Compare July 23, 2020 19:13

emgre added 2 commits July 27, 2020 13:52

Merge remote-tracking branch 'remotes/upstream/master' into bugfix-is…

28c2d7c

…sue-2641

Update loom to 3.4.5.

d5507b3

emgre changed the title ~~[WIP] Fix #2641, #2535, and shutdown_timeout blocking for no reason~~ Fix #2641, #2535, and shutdown_timeout blocking for no reason Jul 27, 2020

carllerche requested changes Jul 28, 2020

View reviewed changes

tokio/tests/rt_common.rs Show resolved Hide resolved

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

Prevent worker thread queue from growing without limit.

6643d2c

carllerche requested changes Jul 28, 2020

View reviewed changes

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

emgre force-pushed the bugfix-issue-2641 branch from b7780dd to 556b05b Compare July 28, 2020 20:32

Keep track of worker threads with slab instead of a Vec.

602dd34

emgre force-pushed the bugfix-issue-2641 branch from 556b05b to 602dd34 Compare July 28, 2020 21:05

carllerche requested changes Jul 28, 2020

View reviewed changes

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

Slab consumption optimisation.

b2e5a06

carllerche approved these changes Jul 29, 2020

View reviewed changes

carllerche merged commit 646fbae into tokio-rs:master Jul 29, 2020

emgre mentioned this pull request Aug 28, 2020

Update to tokio 1.0 stepfunc/dnp3#9

Closed

Darksonn removed the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Nov 10, 2020

Darksonn mentioned this pull request Nov 10, 2020

Prepare 0.2.23 release #3114

Merged

ajfrantz mentioned this pull request Dec 24, 2020

Runtime::shutdown_timeout unnecessarily blocks until the timeout expires #2314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2641, #2535, and `shutdown_timeout` blocking for no reason #2649

Fix #2641, #2535, and `shutdown_timeout` blocking for no reason #2649

emgre commented Jul 8, 2020 •

edited

Loading

emgre commented Jul 8, 2020

Darksonn left a comment

Darksonn commented Jul 20, 2020

emgre commented Jul 20, 2020

hawkw left a comment

carllerche commented Jul 20, 2020

jadamcrain commented Jul 21, 2020

emgre commented Jul 23, 2020

hawkw commented Jul 23, 2020 •

edited

Loading

emgre commented Jul 23, 2020

Darksonn commented Jul 27, 2020

emgre commented Jul 27, 2020 •

edited

Loading

emgre commented Jul 28, 2020

carllerche left a comment

carllerche commented Jul 28, 2020

carllerche left a comment

carllerche left a comment

Fix #2641, #2535, and shutdown_timeout blocking for no reason #2649

Fix #2641, #2535, and shutdown_timeout blocking for no reason #2649

Conversation

emgre commented Jul 8, 2020 • edited Loading

emgre commented Jul 8, 2020

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn commented Jul 20, 2020

emgre commented Jul 20, 2020

hawkw left a comment

Choose a reason for hiding this comment

carllerche commented Jul 20, 2020

jadamcrain commented Jul 21, 2020

emgre commented Jul 23, 2020

hawkw commented Jul 23, 2020 • edited Loading

emgre commented Jul 23, 2020

Darksonn commented Jul 27, 2020

emgre commented Jul 27, 2020 • edited Loading

emgre commented Jul 28, 2020

carllerche left a comment

Choose a reason for hiding this comment

carllerche commented Jul 28, 2020

carllerche left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Fix #2641, #2535, and `shutdown_timeout` blocking for no reason #2649

Fix #2641, #2535, and `shutdown_timeout` blocking for no reason #2649

emgre commented Jul 8, 2020 •

edited

Loading

hawkw commented Jul 23, 2020 •

edited

Loading

emgre commented Jul 27, 2020 •

edited

Loading