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

Fix memory leak/growth when creating many runtimes #3564

Merged
merged 20 commits into from
Mar 16, 2021
Merged

Fix memory leak/growth when creating many runtimes #3564

merged 20 commits into from
Mar 16, 2021

Conversation

ipetkov
Copy link
Member

@ipetkov ipetkov commented Feb 28, 2021

  • Rather than using a vector of mpsc channels to dispatch received signals,
    switch to using broadcast channels instead, which automatically clean
    up after themselves on drop without needing a linear scan
  • Bad news is that we have public poll_recv methods on Signal and after a cursory glance I could not figure out how to refactor broadcast::Receiver to have an internal version of poll_recv as we had in mpsc::Receiver, so appreciate any help here!
  • PR is still WIP, but I wanted to start getting feedback

Refs #3550

@ipetkov ipetkov requested a review from carllerche February 28, 2021 00:50
tokio/src/signal/unix.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member

The direction looks good. I think you can use watch here as you are doing broadcast::channel(1). There also is a poll bridge here. You will probably want to vendor a bunch of that in a private tokio module for this signal work.

Thanks 👍

@ipetkov

This comment has been minimized.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Mar 8, 2021
tokio/src/signal/mod.rs Outdated Show resolved Hide resolved
tokio/src/signal/unix.rs Outdated Show resolved Hide resolved
@ipetkov ipetkov marked this pull request as ready for review March 10, 2021 06:09
@ipetkov
Copy link
Member Author

ipetkov commented Mar 10, 2021

@Darksonn @carllerche this is now ready for (final) review!

@Darksonn
Copy link
Contributor

There's a merge conflict.

@ipetkov
Copy link
Member Author

ipetkov commented Mar 10, 2021

Rebased

@ipetkov
Copy link
Member Author

ipetkov commented Mar 11, 2021

Test failure is for test tokio full --unstable (macos-latest), can't find any logs wonder if the runner just died somehow. Going to restart and see if it passes

tokio/src/sync/mod.rs Outdated Show resolved Hide resolved
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! Super close. Code looks good, I had one nit inline.

Could you add a valgrind test? Ex. https://github.com/tokio-rs/tokio/blob/master/.github/workflows/ci.yml#L71-L91

@ipetkov
Copy link
Member Author

ipetkov commented Mar 16, 2021

Added a test, CI is passing now, let me know if anything else is missing!

@ipetkov ipetkov requested a review from carllerche March 16, 2021 16:07
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 👍. I'll let @Darksonn do the final review / merge.

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.

Seems fine to me.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants