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

io: Refactor out usage of Weak in the io handle #4656

Merged
merged 3 commits into from
May 19, 2022
Merged

io: Refactor out usage of Weak in the io handle #4656

merged 3 commits into from
May 19, 2022

Conversation

hidva
Copy link
Contributor

@hidva hidva commented May 5, 2022

Motivation

Fix #4509

Solution

The cycle is broken by making sure nobody can register a new io resources on shutdown and by emptying the driver.

@hidva hidva changed the title io: Refactor out usage of Weak in the io handle [WIP]io: Refactor out usage of Weak in the io handle May 5, 2022
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels May 5, 2022
@Darksonn Darksonn requested a review from LucioFranco May 5, 2022 18:47
@hawkw
Copy link
Member

hawkw commented May 5, 2022

this no longer seems to compile (i think because inner is no longer an Option?): https://github.com/tokio-rs/tokio/runs/6311895497?check_suite_focus=true

@github-actions github-actions bot added the R-loom Run loom tests on this PR label May 6, 2022
@hidva hidva changed the title [WIP]io: Refactor out usage of Weak in the io handle io: Refactor out usage of Weak in the io handle May 6, 2022
@hidva
Copy link
Contributor Author

hidva commented May 6, 2022

@LucioFranco Please take a look, I have done a benchmark with monoio-benchmark, and there is no performance fallback.

@LucioFranco
Copy link
Member

@carllerche can you take a look at this?

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looks like a good start! I would love some other eyes on this as well though cc @carllerche

tokio/src/io/driver/mod.rs Outdated Show resolved Hide resolved
tokio/src/io/driver/mod.rs Outdated Show resolved Hide resolved
tokio/src/io/driver/mod.rs Show resolved Hide resolved
tokio/src/runtime/metrics/runtime.rs Show resolved Hide resolved
Copy link

@renancloudwalk renancloudwalk left a comment

Choose a reason for hiding this comment

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

👍

tokio/src/io/driver/mod.rs Show resolved Hide resolved
@hidva hidva requested a review from LucioFranco May 9, 2022 13:15
Comment on lines 283 to 287
impl Handle {
pub(crate) fn metrics(&self) -> &IoDriverMetrics {
&self.inner.metrics
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The impl Handle is currently indented by seven spaces rather than eight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Sorry, because I see the last commit is indented like this.

fn shutdown(&mut self) {}
fn shutdown(&mut self) {
if self.inner.shutdown() {
self.resources.for_each(|io| {
Copy link
Member

Choose a reason for hiding this comment

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

If this statement is commented out, do tests fail? i.e. if there is a memory leak, is it caught my CI? If not, we should add one.

Copy link
Contributor Author

@hidva hidva May 16, 2022

Choose a reason for hiding this comment

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

Yes.

First, we have to comment out clear_wakers().

Second, if we commented out io.shutdown() here, we'd get a memory leak, otherwise we wouldn't.

$ valgrind --error-exitcode=1 --leak-check=full --show-leak-kinds=all ./target/debug/test-mem
==756352== LEAK SUMMARY:
==756352==    definitely lost: 96 bytes in 1 blocks
==756352==    indirectly lost: 20,856 bytes in 35 blocks
==756352==      possibly lost: 0 bytes in 0 blocks
==756352==    still reachable: 9,952 bytes in 69 blocks
==756352==         suppressed: 0 bytes in 0 blocks
==756352==
==756352== For lists of detected and suppressed errors, rerun with: -s
==756352== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

(It looks like we don't need to call clear_wakers() in Drop for Registration anymore.

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 for doing this. The general direction looks good to me. I would like to double check that we have CI tests that check for memory leaks (valgrind). Given that a bug here will result in a mem leak, it would be good to check :) I left a note inline where, if commented, I would hope CI would fail.

Thanks!

@carllerche
Copy link
Member

There are a few places valgrind is used in CI:

I wonder if we could run our entire CI suite w/ valgrind...

@Darksonn
Copy link
Contributor

Regarding leak checks, see the recently merged #4696. You may want to merge in master to get this step in the PR's CI.

@hidva
Copy link
Contributor Author

hidva commented May 16, 2022

Regarding leak checks, see the recently merged #4696. You may want to merge in master to get this step in the PR's CI.

done

@Darksonn Darksonn merged commit 931a777 into tokio-rs:master May 19, 2022
@Darksonn
Copy link
Contributor

Thanks!

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 11, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.18.2` -> `1.19.1` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.18.2` -> `1.19.1` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.19.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.1)

[Compare Source](tokio-rs/tokio@tokio-1.19.0...tokio-1.19.1)

##### 1.19.1 (June 5, 2022)

This release fixes a bug in `Notified::enable`. ([#&#8203;4747])

[#&#8203;4747]: tokio-rs/tokio#4747

### [`v1.19.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.19.0)

[Compare Source](tokio-rs/tokio@tokio-1.18.2...tokio-1.19.0)

##### 1.19.0 (June 3, 2022)

##### Added

-   runtime: add `is_finished` method for `JoinHandle` and `AbortHandle` ([#&#8203;4709])
-   runtime: make global queue and event polling intervals configurable ([#&#8203;4671])
-   sync: add `Notified::enable` ([#&#8203;4705])
-   sync: add `watch::Sender::send_if_modified` ([#&#8203;4591])
-   sync: add resubscribe method to broadcast::Receiver ([#&#8203;4607])
-   net: add `take_error` to `TcpSocket` and `TcpStream` ([#&#8203;4739])

##### Changed

-   io: refactor out usage of Weak in the io handle ([#&#8203;4656])

##### Fixed

-   macros: avoid starvation in `join!` and `try_join!` ([#&#8203;4624])

##### Documented

-   runtime: clarify semantics of tasks outliving `block_on` ([#&#8203;4729])
-   time: fix example for `MissedTickBehavior::Burst` ([#&#8203;4713])

##### Unstable

-   metrics: correctly update atomics in `IoDriverMetrics` ([#&#8203;4725])
-   metrics: fix compilation with unstable, process, and rt, but without net ([#&#8203;4682])
-   task: add `#[track_caller]` to `JoinSet`/`JoinMap` ([#&#8203;4697])
-   task: add `Builder::{spawn_on, spawn_local_on, spawn_blocking_on}` ([#&#8203;4683])
-   task: add `consume_budget` for cooperative scheduling ([#&#8203;4498])
-   task: add `join_set::Builder` for configuring `JoinSet` tasks ([#&#8203;4687])
-   task: update return value of `JoinSet::join_one` ([#&#8203;4726])

[#&#8203;4498]: tokio-rs/tokio#4498

[#&#8203;4591]: tokio-rs/tokio#4591

[#&#8203;4607]: tokio-rs/tokio#4607

[#&#8203;4624]: tokio-rs/tokio#4624

[#&#8203;4656]: tokio-rs/tokio#4656

[#&#8203;4671]: tokio-rs/tokio#4671

[#&#8203;4682]: tokio-rs/tokio#4682

[#&#8203;4683]: tokio-rs/tokio#4683

[#&#8203;4687]: tokio-rs/tokio#4687

[#&#8203;4697]: tokio-rs/tokio#4697

[#&#8203;4705]: tokio-rs/tokio#4705

[#&#8203;4709]: tokio-rs/tokio#4709

[#&#8203;4713]: tokio-rs/tokio#4713

[#&#8203;4725]: tokio-rs/tokio#4725

[#&#8203;4726]: tokio-rs/tokio#4726

[#&#8203;4729]: tokio-rs/tokio#4729

[#&#8203;4739]: tokio-rs/tokio#4739

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1394
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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.

io: Refactor out usage of Weak in the io handle
6 participants