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

std::future: Document that each Future (Stream, etc) must only be used by one task at a time #66709

Open
sdroege opened this issue Nov 24, 2019 · 4 comments

Comments

@sdroege
Copy link
Contributor

@sdroege sdroege commented Nov 24, 2019

See snapview/tokio-tungstenite#68 (comment) for context.

I couldn't find anything in the docs about this (maybe I just missed it), but current futures executors / async runtimes only allow remembering one waker per future/etc. So if you use the same in multiple tasks, one of the tasks would potentially never be woken up.

Allowing only one makes sense for performance reasons, so seems like a good idea.

This should be documented somewhere to make this constraint clear.

@tmandry

This comment has been minimized.

Copy link
Contributor

@tmandry tmandry commented Mar 17, 2020

I think the constraint you're talking about is that multiple tasks cannot expect to be woken by the same Waker. Is that right?

If so, in what kind of situation does that arise? Normally you get a Waker from the Context your future is getting polled with, and that waker is specific to the task. Is this a situation where you have a shared data structure between many tasks, and wanted to use a single waker instead of a list of wakers?

@sdroege

This comment has been minimized.

Copy link
Contributor Author

@sdroege sdroege commented Mar 17, 2020

In this case the situation was the websocket implementation in tokio-tungstenite. It implements the websocket protocol on top of some AsyncRead + AsyncWrite. The socket can then be split into a receiver (Stream) and sender (Sink), which can be handled by separate tasks.

Now the problem is that the receiver can receive a Ping message, for which it then automatically sends a Pong response. This then means that the underlying AsyncWrite can be used from two different tasks, and both tasks might be waiting on it at the same time. The TcpStream provided by tokio, async-std, etc. however only allow for a single Waker (and thus a single task) to ever wait for it to become ready again.

The solution in tokio-tungstenite now is to pass a proxy Waker to the AsyncWrite that allows to store and dispatch to (up to) two actual Wakers.

Having this explicitly documented somewhere would've prevented (hopefully) quite some time to debug weird behaviour :)

@Matthias247

This comment has been minimized.

Copy link
Contributor

@Matthias247 Matthias247 commented Mar 22, 2020

I think that is a library limitation, not a Future one. Most libraries already enforce that their types can only be used by a single task at a time by required a &mut self reference. Of course you could start polling there, and then hand over the poll to another task - which would lead the first task never to get notified. But I can't see a good reason why anyone would do it and expect it to work.

There are types in the ecosystem which allow concurrent consumption from multiple tasks through &self access. E.g. the queues in futures-intrusive. But those would still have a single Future per consuming task - it is just possible to create multiple Futures for the same operation.

Regarding tokio-tungestenite: I would recommend to model the library in a way where it has a decided read and write task. Both tasks should have fixed ownership over the reading/writing half of the socket. The read task can enqueue sending a Pong - e.g. through a Channel. Or by locking a synchronous Mutex, placing the ping request there, unlocking it, and notifying the write task to wake up. The read task can even wait until the pong is sent, by also providing a handle to the write task which allows to wake it up again. A oneshot channel works, but in order to reduce allocations you can also use a reusable channel or ManualResetEvent.

@sdroege

This comment has been minimized.

Copy link
Contributor Author

@sdroege sdroege commented Mar 23, 2020

Regarding tokio-tungestenite: I would recommend to model the library in a way where it has a decided read and write task.

That would make the library bound to a single async runtime unfortunately. There's no generic API for spawning tasks yet, so what is currently done seems like the best solution possible.

I think that is a library limitation, not a Future one.

That's definitely true, like the current solution in tokio-tungstenite shows. But as this affects a majority of cases for now it might make sense to still document this somewhere? Basically saying that right now it's only guaranteed that one waker will be woken up, but for specific types (like the mutexes) multiple wakers might be possible. Basically a "be careful" note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.