Merged
Conversation
…nt of Registration
… add more unit tests
a0c8911 to
04950cd
Compare
Contributor
Author
|
@yoshuawuyts and I went through the majority of these changes together and gave it a verbal review to merge. |
This was referenced Dec 28, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In short: the runtime's reactor has been redesigned so that a Wasi
Pollablecan be reused to await for more than one readiness event.Prior to this PR, the only way to wait for a pollable is with
Reactor'sasync fn wait_for(&self, pollable: Pollable). (This method still exists for compatibility, but will dissapear in future PRs after the rest of the repo is ported.) Because this method takes ownership of thePollable, it can only be used to await for readiness once per Pollable. In Wasi, a Pollable can signal readiness for the entire lifetime of the parent resource. Wstd's failure to reuse results in lots of unnecessary churn callingsubscribeand[drop]pollableimport calls for each await.The new
Reactormethodfn schedule(&self, pollable: Pollable) -> AsyncPollablegives a newAsyncPollabletype, which implementsCloneand has a (notional)async fn wait_for(&self)method. This means that eachPollablecan now, upon being transferred to theReactorviaschedule, be reused in many futures given byAsyncPollable::wait_for. The legacyReactor::wait_foris now implemented with aReactor::scheduleand immediateAsyncPollable::wait_for.One knock-on effect is that a
Pollableremains owned by theReactorfor as long as there is a liveAsyncPollablepointing to it. However, its liveness doesn't necessarily respond to the user's program awaiting on its readiness. So, rather than callingwasi::io::poll::pollon the entire set of owned Polllables, we now have to track which Pollable has Future being polled, and only poll on that subset. This leads to a second layer of registration, private to the internals of this crate, calledWaitee, wherein any of theWaitForfutures given by theAsyncPollable::wait_formethod registers aWaiterwith theReactorduring the firstFuture::pollcall (if it does not immediately result in Ready). To handle cancellation properly, the Waitee is deregistered when the Future is dropped.A further knock-on effect of this change is that it is no longer appropriate for a
Future::pollcall to callReactor::schedule, including via the legacyReactor::wait_for, because that will produce a registration of thePollablewhich will only live as long as thepollcall. This meant that theTimerhad to be restructured to hold a long-livedAsyncPollable, and have a notionalasync fn wait(&self)that returns a Future, rather than having its ownimpl Future.