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: swallow panics in drop(join_handle) #4430

Merged
merged 3 commits into from Jan 28, 2022

Conversation

BraulioVM
Copy link
Contributor

Fixes #4412

Motivation

From the issue description:

In Tokio, panics are generally caught and not passed resumed when
dropping the JoinHandle, however when dropping the JoinHandle of a task
that has already completed, that panic can propagate to the user who
dropped the JoinHandle.

Solution

Just swallow the panic? I've seen other places in tokio where that happens, but I'm open to hearing
if you had other ideas for this.

fixes tokio-rs#4412

From the issue description:

> In Tokio, panics are generally caught and not passed resumed when
dropping the JoinHandle, however when dropping the JoinHandle of a task
that has already completed, that panic can propagate to the user who
dropped the JoinHandle.

This PR fixes that.
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Jan 27, 2022
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Jan 27, 2022
@Darksonn
Copy link
Contributor

And indeed I think we should swallow the panic here. The reason is that panics in tasks are delivered to the user via the JoinHandle, so if the user has thrown the JoinHandle away, then they have told us they don't want the panic.

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks!

@BraulioVM
Copy link
Contributor Author

Done?

@Darksonn
Copy link
Contributor

Yeah, I didn't merge immediately because CI was still running.

@Darksonn Darksonn merged commit 111dd66 into tokio-rs:master Jan 28, 2022
hawkw added a commit that referenced this pull request Feb 15, 2022
# 1.16.2 (February 15, 2022)

This release updates the minimum supported Rust version (MSRV) to 1.49,
the `mio` dependency to v0.8, and the (optional) `parking_lot`
dependency to v0.12. Additionally, it contains several bug fixes, as
well as internal refactoring and performance improvements.

### Fixed

- time: prevent panicking in `sleep` with large durations ([#4495])
- time: eliminate potential panics in `Instant` arithmetic on platforms
  where `Instant::now` is not monotonic ([#4461])
- io: fix `DuplexStream` not participating in cooperative yielding
  ([#4478])
- rt: fix potential double panic when dropping a `JoinHandle` ([#4430])

### Changed

- update minimum supported Rust version to 1.49 ([#4457])
- update `parking_lot` dependency to v0.12.0 ([#4459])
- update `mio` dependency to v0.8 ([#4449])
- rt: remove an unnecessary lock in the blocking pool ([#4436])
- rt: remove an unnecessary enum in the basic scheduler ([#4462])
- time: use bit manipulation instead of modulo to improve performance
  ([#4480])
- net: use `std::future::Ready` instead of our own `Ready` future
  ([#4271])
- replace deprecated `atomic::spin_loop_hint` with `hint::spin_loop`
  ([#4491])
- fix miri failures in intrusive linked lists ([#4397])

### Documented

- io: add an example for `tokio::process::ChildStdin` ([#4479])

### Unstable

The following changes only apply when building with `--cfg
tokio_unstable`:

- task: fix missing location information in `tracing` spans generated by
  `spawn_local` ([#4483])
- task: add `JoinSet` for managing sets of tasks ([#4335])
- metrics: fix compilation error on MIPS ([#4475])
- metrics: fix compilation error on arm32v7 ([#4453])

[#4495]: #4495
[#4461]: #4461
[#4478]: #4478
[#4430]: #4430
[#4457]: #4457
[#4459]: #4459
[#4449]: #4449
[#4462]: #4462
[#4436]: #4436
[#4480]: #4480
[#4271]: #4271
[#4491]: #4491
[#4397]: #4397
[#4479]: #4479
[#4483]: #4483
[#4335]: #4335
[#4475]: #4475
[#4453]: #4453
hawkw added a commit that referenced this pull request Feb 16, 2022
# 1.17.0 (February 16, 2022)

This release updates the minimum supported Rust version (MSRV) to 1.49,
the `mio` dependency to v0.8, and the (optional) `parking_lot`
dependency to v0.12. Additionally, it contains several bug fixes, as
well as internal refactoring and performance improvements.

### Fixed

- time: prevent panicking in `sleep` with large durations ([#4495])
- time: eliminate potential panics in `Instant` arithmetic on platforms
  where `Instant::now` is not monotonic ([#4461])
- io: fix `DuplexStream` not participating in cooperative yielding
  ([#4478])
- rt: fix potential double panic when dropping a `JoinHandle` ([#4430])

### Changed

- update minimum supported Rust version to 1.49 ([#4457])
- update `parking_lot` dependency to v0.12.0 ([#4459])
- update `mio` dependency to v0.8 ([#4449])
- rt: remove an unnecessary lock in the blocking pool ([#4436])
- rt: remove an unnecessary enum in the basic scheduler ([#4462])
- time: use bit manipulation instead of modulo to improve performance
  ([#4480])
- net: use `std::future::Ready` instead of our own `Ready` future
  ([#4271])
- replace deprecated `atomic::spin_loop_hint` with `hint::spin_loop`
  ([#4491])
- fix miri failures in intrusive linked lists ([#4397])

### Documented

- io: add an example for `tokio::process::ChildStdin` ([#4479])

### Unstable

The following changes only apply when building with `--cfg
tokio_unstable`:

- task: fix missing location information in `tracing` spans generated by
  `spawn_local` ([#4483])
- task: add `JoinSet` for managing sets of tasks ([#4335])
- metrics: fix compilation error on MIPS ([#4475])
- metrics: fix compilation error on arm32v7 ([#4453])

[#4495]: #4495
[#4461]: #4461
[#4478]: #4478
[#4430]: #4430
[#4457]: #4457
[#4459]: #4459
[#4449]: #4449
[#4462]: #4462
[#4436]: #4436
[#4480]: #4480
[#4271]: #4271
[#4491]: #4491
[#4397]: #4397
[#4479]: #4479
[#4483]: #4483
[#4335]: #4335
[#4475]: #4475
[#4453]: #4453
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 Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JoinHandle destructor should not panic when dropping output
2 participants