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

Add tokio02-style mpsc sender #3105

Closed
wants to merge 30 commits into from
Closed

Conversation

MikailBag
Copy link
Contributor

@MikailBag MikailBag commented Nov 6, 2020

Motivation

Some users want to have poll-based API for mpsc channels.

Solution

Add some amount of unsafe.

Ready(Permit<'static, T>),
/// We are in process of acquiring a permit
// ALERT: contained future contains self-reference to the sender.
Acquire(Pin<Box<dyn Future<Output = Result<Permit<'static, T>, SendError<()>>>>>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This boxing can be avoided when type_alias_impl_trait finally lands.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, if this was implemented in tokio rather than in tokio-util, this could just use the internal semaphore implementation's Acquire future. But, I don't think that's going to ever become public API, so this would need to be in tokio proper.

@@ -1 +1 @@

mod mpsc;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that cancellation token tests are just ignored currently.

tokio-util/src/sync/mpsc.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/mpsc.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/mpsc.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/mpsc.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/mpsc.rs Outdated Show resolved Hide resolved
tokio-util/src/sync/tests/mpsc.rs Outdated Show resolved Hide resolved
Comment on lines 113 to 115
run: cargo miri test --features rt,rt-multi-thread,sync task
run: |
cargo miri test --features rt,rt-multi-thread,sync task
cargo miri test -p tokio-util 'sync::tests::mpsc'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but we should have a miri test that calls is_ready and clone while the state is in either Ready or Acquire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it seems that there is no way to poll futures under MIRI.
Of course, I plan to write some less trivial tests but there is no way to verify them in MIRI in the near future.

@taiki-e taiki-e added A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-sync Module: tokio/sync labels Nov 7, 2020
@coolreader18
Copy link

I think the CI failure is due to tokio-util not explicitly depending on the tokio/sync feature. Also, I have a use case for this feature, in wrapping a Sender in an AsyncWrite.

@MikailBag
Copy link
Contributor Author

fmt job fails on code that was not changed by this PR.
It seems that rustfmt somehow changed its behavior.

@taiki-e
Copy link
Member

taiki-e commented Nov 20, 2020

Format issue was fixed by #3160. It can be fixed by merging master.

@coolreader18
Copy link

I might have misunderstood, but I think making T not require 'static doesn't necessarily mean adding another 'a lifetime parameter. Cause the permit is supposed to be a self reference, right? So what lifetime would make sense for 'a other than 'static?

@MikailBag
Copy link
Contributor Author

Well, I have to use some lifetime for Permit<'_, T>.
If I use 'static, it rules out all non-'static types.
Of course, for owned message types 'a should be selected as 'static.

Do you have any ideas on how to make it easier to use?

@coolreader18
Copy link

Oh, I see... the variance of Permit requires that T: 'a, and 'a can't be 'static if T: !'static. That is really tricky, I don't think I have an easy solution.

@Darksonn
Copy link
Contributor

I don't think we can get around that without implementing it in Tokio proper.

@MikailBag
Copy link
Contributor Author

Actually, it seems that we can have a generic wrapper that can poll-ify arbitrary async fn. I'll check if I'm right.

@MikailBag
Copy link
Contributor Author

Committed new, generic pollifier.

@coolreader18
Copy link

Is there any update on this?

@MikailBag
Copy link
Contributor Author

AFAIK this PR needs some review

@Darksonn
Copy link
Contributor

Just as a status-update, I don't think I will get around to reviewing this until after Tokio 1.0 is released later this month. It involves some serious unsafe code to soundly build a self-referential struct, and there are some things I would have to try out in miri before I am happy with it.

@Darksonn
Copy link
Contributor

Closing this, as I have just opened #3490, which implements this without unsafe, while still avoiding allocating for every sent item. The new PR also avoids the lifetime issues that this PR runs into.

@Darksonn Darksonn closed this Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants