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

Context and Waker might be accidentally `Sync` #66481

Open
Matthias247 opened this issue Nov 16, 2019 · 12 comments

Comments

@Matthias247
Copy link
Contributor

@Matthias247 Matthias247 commented Nov 16, 2019

One issue that came up in the discussion here is that the Context type implements Send + Sync.

This might have been introduced accidentally. Send probably does not matter at all, given that users will only observe a Context by reference. However Sync has the impliciation that we will not be able to add non thread-safe methods to Context - e.g. in order to optimize thread-local wake-ups again.

It might be interesting to see whether Send and Sync support could be removed from the type. Unfortunately that is however a breaking change - even though it is not likely that any code currently uses Context out of the direct poll() path.

In a similar fashion it had been observed in the implementation of #65875 (comment) that the Waker type is also Send and Sync. While it had been expected for Send- given that Wakers are used to wake-up tasks from different threads, it might not have been for Sync. One downside of Wakers being Sync is that it prevents optimizations. E.g. in linked ticket the original implementation contained an optimization that while a Waker was not cloned (and thereby equipped with a different vtable) it could wake-up the local eventloop again by just setting a non-synchronized boolean flag in the current thread. However given that &Waker could be transferred to another thread, and .wake_by_ref() called from there even within the .poll() context - this optmization is invalid.

Here it would also be interesting if the Sync requirement could be removed. I expect the amount of real-world usages to be in the same ballpark as sending &Context across threads - hopefully 0.
But it's again a breaking change 😢

cc @Ralith , @Nemo157 , @cramertj , @withoutboats

@withoutboats

This comment has been minimized.

Copy link
Contributor

@withoutboats withoutboats commented Nov 17, 2019

Even if this weren't a breaking change, I'd want to see some really good evidence these optimizations would matter for real world code before choosing to make these types non-Sync. I don't think getting rid of an atomic update in thread::block_on is a compelling example. Basically, in my view the current API is intentional and correct.

@withoutboats withoutboats removed the T-lang label Nov 17, 2019
@Ralith

This comment has been minimized.

Copy link
Contributor

@Ralith Ralith commented Nov 17, 2019

Context was introduced to leave room for future unforeseen requirements, and being Sync restricts its usefulness as such, which is unfortunate. I can see a case for Waker: Sync since we have wake_by_ref, but it's difficult to imagine a case where it makes sense to be accessing a single Context from multiple threads concurrently.

@worktycho

This comment has been minimized.

Copy link

@worktycho worktycho commented Nov 17, 2019

I have an example of where the current API is resulting in performance issues: I am trying to implement a combined Executor/Reactor around a winit event loop. To wake up the event loop from a different thread we have to post a message to the event loop which can be a non-trivial operation. But on the same thread, we know that we don't need to wake up the eventloop, so we can use much cheaper mechanisms like an atomic bool.

@withoutboats

This comment has been minimized.

Copy link
Contributor

@withoutboats withoutboats commented Nov 17, 2019

@worktycho But that would be true if wakers were just Send also, which was an intentional part of the simplification from the previous design.

@Ralith

This comment has been minimized.

Copy link
Contributor

@Ralith Ralith commented Nov 17, 2019

I think the point is that Context being Send + Sync precludes the otherwise noninvasive restoration of a more efficient LocalWaker through an additional method on Context.

@cmcq

This comment has been minimized.

Copy link

@cmcq cmcq commented Nov 20, 2019

Here is an example of what would go wrong with restoring LocalWaker: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=37911ffa78b9bfa3faed9afc73648db5

(So even a hypothetical get_local_waker(&mut self) can be used from another thread.)

@Matthias247

This comment has been minimized.

Copy link
Contributor Author

@Matthias247 Matthias247 commented Dec 1, 2019

Even if this weren't a breaking change, I'd want to see some really good evidence these optimizations would matter for real world code before choosing to make these types non-Sync. I don't think getting rid of an atomic update in thread::block_on is a compelling example.

I don't think it should be the judgement of the libs team (or any of us) to determine whether something is good enough and not needs to be further optimized for any use-case. Rusts goal as a language is to enable zero cost abstractions. This requires in my opinion to not be opinionated on any design choices which have a performance/footprint impact. I think some of the decisions that have been taken in the async/await world however are opinionated, and will have an impact on use-cases that currently are still better implemented with different mechanisms.

I do not really want to elaborate on anything more particular, because I fear this would bring the discussion back to a arguing whether any X% performance degradation is still good enough. That is an OK decision to have for a software project whichs needs to decide whether Rusts async/await support is good enough for them, and whether they have to apply workarounds or not. But it's not a discussion which will move the issue here any more forward, because for yet another project the outcoming of the discussion might be different.

PS: I do think it's okay and good if libraries like tokio or async-std are being more opinionated about what threading models they support, and they might sacrifice performance gains in rare usage scenarios for easy of use. But core and language features are different, and we expect people to use them also for niche use-cases (e.g. there is certainly a lot of cool evaluation going on with the use of async/await in embedded contexts or kernels - where requirements might be very different than for a webserver based on tokio).

@withoutboats

This comment has been minimized.

Copy link
Contributor

@withoutboats withoutboats commented Dec 3, 2019

I don't think it should be the judgement of the libs team (or any of us) to determine whether something is good enough and not needs to be further optimized for any use-case. Rusts goal as a language is to enable zero cost abstractions.

This is a trade off: Context is either Sync or it isn't, some users benefit from one choice (they can use references to Context as threadsafe) and some users benefit from the other choice (they can have nonthreadsafe constructs inside the Context construct). Ultimately the libs team has to decide one way or the other on these points in the API where there is a trade off between thread safety and not.

However, this decision has already been made and it would be a breaking change to change it, so this discussion is moot.

@kabergstrom

This comment has been minimized.

Copy link

@kabergstrom kabergstrom commented Dec 3, 2019

(they can use references to Context as threadsafe)

Do you have an example where this is actually done within the ecosystem, or a use-case for it? As far as I can tell, this would require spawning a thread using something like crossbeam_utils::thread::scope and capturing the Context from within a Future::poll?

@withoutboats

This comment has been minimized.

Copy link
Contributor

@withoutboats withoutboats commented Dec 3, 2019

A common way a user could depend on a type being Sync is to store it in an Arc and send it across threads. This isn't very likely for Context but it is a very reasonable pattern for Waker.

@withoutboats

This comment has been minimized.

Copy link
Contributor

@withoutboats withoutboats commented Dec 3, 2019

I think the point is that Context being Send + Sync precludes the otherwise noninvasive restoration of a more efficient LocalWaker through an additional method on Context.

I think its a fair point that we didnt intentionally make context send and sync, and that this precludes adding more fields to context that are not send and sync, and that since context is just a buffer for future changes, it would probably have been better to make it non-threadsafe. But now its a breaking change. If crater showed no regressions and there's no known patterns it nullifies, I would be open to changing this about Context personally, but I am somewhat doubtful the libs team as a whole would approve making a breaking change to support hypothetical future extensions.

If anyone on the thread wants to pursue a change to context (not waker), they should get crater results as the next step of the conversation.

@Matthias247

This comment has been minimized.

Copy link
Contributor Author

@Matthias247 Matthias247 commented Dec 9, 2019

A common way a user could depend on a type being Sync is to store it in an Arc and send it across threads. This isn't very likely for Context but it is a very reasonable pattern for Waker.

There is no discussion about Waker being Send or not - it obviously has to be. The question is purely about Sync. And in order to do what you describe you have to store an Arc<&Waker> and send it somewhere. Which is very doubtful to happen, due to the not very useful lifetime and due to Waker already being an Arc like thing internally.

As @kabergstrom mentioned, the most likely way to see this behavior is some scoped thread API being used inside poll - or people doing some very advanced synchronization against an IO driver running in another thread. For all those there are certainly better ways than to rely on Waker being Sync. E.g. to return a flag from the synchronized section and call waker.wake_by_ref() from the original poll() thread when done. Or to .clone() and send the Waker if there is doubt whether it needs to be persisted somewhere else.

And yes, the impact on Context is even bigger. It was meant as an extension point. But we can not add any functions to it which are not thread-safe. E.g. if we want to add methods which spawn another task on the same thread as the current executor thread, and which allows for spawning !Send futures (like Tokios LocalSet) - we would have an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.