Add a minimal Futures executor #65875
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This change adds `std::thread::block_on_future`, which represents a minimal Futures executor. It is modelled after futures-rs `futures::executor::block_on`, which blocks the current thread until the Future had been driven to completion.
Matthias247
force-pushed
the
block_on_future
branch
from
128dad7
to
dcd2a01
Compare
| // we would need to wrap the complete `Thread` object in another `Arc` by | ||
| // adopt the following line to: | ||
| // `let arc_thread = Arc::new(current());` | ||
| let arc_thread_inner = current().inner; |
There was a problem hiding this comment.
Both &Context and &Waker are Send, so they might be send to another thread
(however unlikely it is to happen in practice). Thus it isn't a valid
optimization to delay obtaining the current thread.
There was a problem hiding this comment.
Oh, that's a bummer. I somehow missed that Waker is Sync - didn't remember we made it that way. Guess I will remove the optimization again.
LocalWaker would also have been nice for this use-case, but that's also gone.
|
I pushed a change which removes the optimizations, since as pointed out by @tmiasko they were not allowed due to |
As it was pointed out in the review, the initial `Waker` for `block_on_future` was not holding up the `Sync` guarantees. If the `Waker` reference had been passed to another thread and cloned there, the cloned threadsafe `Waker` would have captured the wrong `Thread` handle. This change removes the optimization. A threadsafe `Waker` is now immediately created.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Matthias247
force-pushed
the
block_on_future
branch
from
ed14268
to
7627b0f
Compare
cramertj
added
the
T-libs-api
|
I don't feel super strongly about this, but I wouldn't expect this to be used in any real applications, and would expect this to be mostly useful for demo code. I guess that's a question for @rust-lang/libs about whether they want to include something like that in std. |
|
block_on is useful when creating blocking APIs over async APIs, but that's a pretty niche use case. |
|
I think there are 2 use cases for this:
For the latter there might however be some restrictions. Eg polling Futures that only work when polled from a certain executor (eg the Tokio one) will not work. It would work, if those frameworks separate the executor from reactors so that IO could also be performed from an outside executor like this one. This would in general be nice to have for improving interoperability in the async ecosystem. However since it might have performance implications we might need to live with the fact that not all async fns might work here. But still - some will work - and eg blocking on an async Channel to retrieve a notification from async subtasks can already be helpful and allow to build bridging APIs. |
|
I feel like it is too soon to do this. I have no strong opinion about whether it should be included in std, but that's because we haven't received enough feedback from users yet to make that decision confidently. |
|
Even having this in a permanently unstable fashion in std would be useful for just trying things out wrt hacking on the compiler and whatnot. |
|
I don’t have an opinion on this particular API, but I don’t think we should plan for anything intended to have external users to be permanently unstable. Instability should be a transition state until we either stabilize something, or deprecate and then remove it. (Though it’s ok if that transition takes a long time or is blocked on some other decision.) |
This is a user-feedback :-) I had and have requirements to use async libraries from within plain old threads. If you are interested in other data-points that provide it's helpful:
I also don't think the API is controversial and will get superseeded by something else more appealing, since it's super minimal. I could expect we might want a more advanced API later on, that e.g. allows spawning or that allows to run some methods in each eventloop integration (e.g. to drive timers). But I agree those might need to see more requirements and more design work. Or we might want to integrate a configurable cancellation hook for the future that gets polled - but that again could be exposed via another API. May this is a bit far stretched, but I think this might also have a positive impact on interoperability between runtimes in the ecosystem: If a stdlib function allows to block on futures it might raise the priority for runtime authors to allowing polling and driving their |
|
This is already available through the very fundamental futures library (not to mention the various runtimes having their own equivalent). Obviously people need a single threaded executor, but as you've already highlighted, there are some subtle design decisions here - mainly the issue around re-entrency (though I also think the exact API surface also matters). I think this should end up in std eventually, but I'd like to gather more feedback about these questions before putting it in. Right now the behavior this PR implements seems to just be your preference (not clear who else has contributed to the decisions around naming and reentrancy you've made here)? That's the problem I'm highlighting: not enough people have used these APIs for us to make these choices at the level of confidence needed for std. |
|
Ping from triage: |
|
I think a brief RFC about the proposed API would be a good next step here. Between the RFC text and the thread we could build a stronger consensus on the re-entrancy question, API name, etc. |
|
wouldn't |
|
I think @Matthias247's choice to put in |
|
Right, this is in
As mentioned earlier, I do not care too much about the name. I just chose I also don't have any strong opinion on the reentrancy question - apart from that I am pretty sure that it can not be supported without any issues (mostly deadlocks) anyway in a general fashion. That fact that I didn't add the reentrancy check is purely due to others having raised concerns on zulip around the use of thread-locals. But I am pretty much ok with either having fail-fast behavior (which requires the thread-local) or declaring reentrancy as undefined behavior (which is what this implementation is doing). |
This statement surprised me but I think its a miscommunication (another way an RFC can help clarify things). We definitely can't declare reentrancy undefined behavior, because undefined behavior means it is the user's obligation to guarantee it never occurs, or all bets are off, including soundness and memory safety. Since this API is safe, it simply can't have undefined behavior. But I don't see any undefined behavior in the code in the PR (maybe my oversight), so I think you probably meant implementation defined behavior - that is, we reserve the right to change the behavior in the future. But our experience has been that the stability guarantees we make for std make it very difficult to change the incidental behavior that users come to rely on. While I could imagine us making a change so that deadlocks become panics maybe (and even that I'm unsure of), it would be very difficult, essentially impossible for us to change the behavior of code that "works," at least in a stable API. So we need to commit to an opinion about reentrancy before we can stabilize this API. |
|
Hey, I came here through @withoutboats post about Global Executors from TWiR, here: https://boats.gitlab.io/blog/post/global-executors/ , I just wanted to chip in my opinion. I am generally against having any kind of global or default executor inside std. I prefer to have some unified interface that will allow me to swap executors (and possibly reactors) seamlessly. My reasoning against Default/Global Executors(1) A global executor can not be the best in everything for everyone, so most of the time users will use their own executor. Different people with different motives will have different ideas of how the global executor should be implemented. I use a special executor for tests (A deterministic single threaded executor, allowing time travel and breaking on deadlocks). I will definitely not use the global executor for my tests, unless it is also deterministic and allows me to break on deadlocks. @Matthias247 wrote:
I think that this is something we can also say about random generators for example. I believe that we don't have a default random generator in std because everyone might have a different idea of what a good random generator is. Some people want fast random generators, and some people want cryptographically secure random, and even about what is really good cryptographic secure random people don't seem to agree. I think that this is a good thing. I have a very fine taste in random generation myself, and I like the fact that I get to pick my random generator myself. (2) If the global executor becomes too convenient, people might actually use it. As a result, I expect that library writers will shove the executor arbitrarily inside their libraries. The problem with a code that contains a hidden executor is that it doesn't play nice with the rest of your async code. For example: I once wanted to have an http server, so I tried This is something I believe could happen very often if a very convenient and "official" global executor exists. Looking at this from the opposite perspective, if there are no "official" global executors at all, I expect that library authors will have to be more agnostic about the executors being used. I was working with async code in python for a very long time, first in Twisted and then in asyncio (previously Tulip). One of the things I disliked the most was the default loop ("loop" is how they call the Executor + Reactor in python). Many python libraries were using the default loop to spawn tasks, which made it very difficult to test any code that touched those libraries. I spent full nights trying to debug code, only to eventually find out that some task of some other library was spawned on a different loop than mine. (3) Once we have an executor inside std, we might not be able to remove it because of backwards compatiblity. Unified spawn interface instead of a Global ExecutorI have been using About the reactor: It is some kind of an invisible beast for me. I have never seen it, and all I know is that if I get things mixed up things end up not working silently, very much not what I'm used to have with Rust. See for example this issue: rust-lang/futures-rs#1285 , when I naively tried to spawn a future that uses a Tokio feature (timer) on a I will be happy to have an abstract reactor interface, allowing me to easily replace it. @withoutboats wrote in the "Global Executors" article:
Is it because spawning means allocation? I agree that library authors should strive to provide the user zero cost interfaces, but I think that sometimes you might want a library to be able to use your Executor. Library exampleLets look at a simple example: an HTTP server library. The final function the library provide could be something like However, the server itself might need to spawn tasks, for example, maybe for WebRTC, or some TLS periodic key replacement, or something else. Then in my opinion a reasonable solution would be to have a function with the following signature: And if at this point we had a Global Executor, things could really go wrong. The library author might be tempted to take the easy route, and just use the Global Executor from std. In that case the function signature will still be Now if I ever want to use my own executor I will not be able to provide it to |
|
@realcr This issue is not about adding global executors to Rust as was mentioned in the blog post, nor around any abstraction which allows to configure an external global executor. The feedback therefore belongs in the internals.rust-lang.org discussion thread around this topic - not here. That discussion thread already captures a variety of feedback, which is partially similar to yours. |
|
@Matthias247 Hi, I was actually not sure where to post my opinion about this issue, the "Global Executors" post only had a link to this PR. That said, I believe that what I wrote is relevant here. To quote what was written at the beginning of this PR:
I think that having something like this inside Is there any corresponding RFC that describes the reasoning behind this PR? If there isn't, maybe we should formulate and RFC explaining the expected changes to |
|
@realcr If you read the discussion on this PR, you'll see that it is blocked on someone creating such an RFC so that we can reach a consensus on the design questions in the block_on interface. However I agree with @Matthias247 that your comment is highly off topic for this PR or such an RFC and has to do with the other parts of my blog posts, which are (as @Matthias247 said) being discussed on the internals thread. |
|
@withoutboats is this blocked on anything? Or just waiting to be reviewed? |
|
@Dylan-DPC In my opinion its a big enough API addition that it's blocked on an RFC. |
|
Thanks. Updating it |
Dylan-DPC-zz
added
S-blocked
|
I am excited to see an RFC for this too. I'll close this PR because I expect the RFC will incorporate the discussion here and we can follow up in a different PR after it plays out. |
|
Note for an eventual RFC/new PR: I believe this implementation has the same lost-wakeup issue with user-code calling |
As disussed on Zulip within wg-async-foundations:
This change adds
std::thread::block_on_future, which represents a minimal Futures executor.It is modelled after futures-rs
futures::executor::block_on, which blocks the current threaduntil the Future had been driven to completion.
The implementation is a bit more efficient than the futures-rs one, since it doesn't require an
additional allocation and some refcount manipulations. It also does not require TLS, since it
makes use already existing TLS information about the current Threads state in the standard
library.
Feature-wise the main difference to the futures-rs implementation is that lacks the reentrancy
detection that is implemented in the futures-rs version. If someone calls
block_onfromwithin an async fn there which already runs inside a
block_onexecutor it will panic. This onewill not, and just spawn another executor. Depending on the code that runs in those executors
this can either work out fine, or lead to a deadlock issue, since the wrong executor handled the
remote
wake()signal. However this isn't deemed as an issue, since wrong usage of APIs canalready lead to deadlocks in other places. Added the deadlock detection back in would require
another TLS variable.
Naming-wise I called this now
block_on_future, but it's totally open for discussion. Just callingit
block_ongives a bit too less information on what it's about. I first hadblcok_on_task- butin the end most users won't know what a task is, and instead only observe it being passed a
Futureas parameter. Something along[a]wait_future()might also work, but then maybe it'smistaken with the
awaitmechanism, which does something different.