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.

Sign up for GitHub

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

Jump to bottom

bugfix: Remove erroneous wake call in SinkWriter #5436

Merged
merged 2 commits into from Feb 7, 2023
Merged

bugfix: Remove erroneous wake call in SinkWriter #5436

merged 2 commits into from Feb 7, 2023

Conversation

funbringer
Copy link
Contributor

Per review comment #5070 (comment):

Adding a wake call here will be a busy loop that consumes 100% CPU
waiting for it to become ready. We shouldn't do that.

Furthermore, according to https://docs.rs/futures-sink/latest/futures_sink/trait.Sink.html#tymethod.poll_ready, poll_ready will make sure that the current task is notified.

Discussion: https://discord.com/channels/500028886025895936/500336346770964480/1072534504981418024

@funbringer
Copy link
Contributor Author

cc @Darksonn

@funbringer funbringer force-pushed the funbringer/tokio-util-sink-writer-remove-wake branch from 78b1da0 to 28d294c Compare February 7, 2023 16:39
@funbringer
bugfix: Remove erroneous wake call in SinkWriter
33e7e91 
Per review comment tokio-rs#5070 (comment):
> Adding a wake call here will be a busy loop that consumes 100% CPU
> waiting for it to become ready. We shouldn't do that.

Furthermore, according to
https://docs.rs/futures-sink/latest/futures_sink/trait.Sink.html#tymethod.poll_ready,
poll_ready will make sure that the current task is notified.

Discussion: https://discord.com/channels/500028886025895936/500336346770964480/1072534504981418024
@funbringer funbringer force-pushed the funbringer/tokio-util-sink-writer-remove-wake branch from 28d294c to 33e7e91 Compare February 7, 2023 16:40
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-io Module: tokio/io labels Feb 7, 2023
@funbringer @Darksonn
Update tokio-util/src/io/sink_writer.rs
8fe91be 
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Darksonn
Darksonn approved these changes Feb 7, 2023
View reviewed changes
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.

@Darksonn Darksonn enabled auto-merge (squash) February 7, 2023 18:26
@Darksonn Darksonn merged commit 5653b45 into tokio-rs:master Feb 7, 2023
@funbringer funbringer deleted the funbringer/tokio-util-sink-writer-remove-wake branch February 7, 2023 18:51
@Darksonn Darksonn mentioned this pull request Feb 9, 2023
Merged
@Darksonn
Copy link
Contributor

Darksonn commented Feb 9, 2023

Note that this bug never appeared in a published release.

@little-arhat
Copy link

Hello! Is it possible to copy discussion from discord (or provide summary) here?

@Darksonn
Copy link
Contributor

Sure, here is the discussion:

@funbringer

Hi there!

Sorry if it's a wrong place to ask questions, but I've been pointed at this piece of code over here. There's a review comment saying that this essentially introduces a busy loop (I tend to agree), but it seems this has never been addressed.

Do you think there's still a problem there?

@Darksonn

UHH. Is that on master?

@funbringer

I believe so.

There you go: https://github.com/tokio-rs/tokio/blob/master/tokio-util/src/io/sink_writer.rs#L111

@Darksonn

Can you submit a PR that removes it?

@funbringer

Sure.

@Darksonn

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
A-tokio-util Area: The tokio-util crate M-io Module: tokio/io
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@funbringer @Darksonn @little-arhat