add stream.forward and future.forward builtins (alternative definition) - #717
Open
lukewagner wants to merge 2 commits into
Open
add stream.forward and future.forward builtins (alternative definition)#717lukewagner wants to merge 2 commits into
stream.forward and future.forward builtins (alternative definition)#717lukewagner wants to merge 2 commits into
Conversation
Co-authored-by: Roman Volosatovs <rvolosatovs@riseup.net>
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.
This PR is based on the great work in #709, but reformulates the definition of
{stream,future}.forwardto avoid the dynamicforwardfield/indirection on streams/futures that was a bit invasive and complex to think through. Instead the definition in this PR mutates the relevant readable and writable ends in-place which ends up being a lot simpler and, iiuc, closer to what an implementation would actually do. To enable this "just mutate the ends" approach in the second commit, the first commit refactors the definition of streams/futures so that streams/futures always have ends (even when the host is the reader and/or writer). This refactoring was a bit invasive, so it felt bigger than something I could reasonably ask for in a review comment in #709. But it ended up becoming a pretty nice simplification for futures/streams overall that reduced lines of code, removed some special cases, and highlighted a spec bug whereDROPPEDwasn't delivered as eagerly as it should have been (included as a WAST test in the refactoring commit that Wasmtime passes).The only actual behavioral difference in this PR vs. #709 that I'm aware of is that there is no trap in the "self-loop" corner case (where the readable and writable ends passed to
{stream,future}.forwardare both attached to the same stream/future); it's just a no-op (the stream/future is no longer alive since both of its ends were just dropped). But if anyone has any good reasons why it should trap, happy to discuss.The refactoring/simplification commit is nice, but touches a lot, so if you're only interested in
{stream,future}.forward, I'd suggest only looking at the second commit. The second commit also includes a bunch of WAST tests for the various interesting semantic cases.PTAL @rvolosatovs