Skip to content

Change stream results to indicate cancellation #498

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

Merged
merged 1 commit into from
Apr 14, 2025
Merged

Conversation

lukewagner
Copy link
Member

As first discussed in #444 and resolving #490, this PR explicitly propagates whether an async stream/future read/write completed or was cancelled. The Buffer.MAX_LENGTH is lowered to 228-1 to match Table.MAX_LENGTH and leave a nice 4-bit space for any future additions.

(Originally, I was going to put the 2-bit "result" field in the MSBs such that Buffer.MAX_LENGTH could be increased if we wanted in the future, but I noticed this is the reverse of the canon lower result, which seemed gratuitously inconsistent. Also, I realized that once we change the CABI to take advantage of multi-return, this issue will go away, since we'll just return two i32s. I'm not wed to this detail, though.)

@lukewagner lukewagner requested a review from alexcrichton April 10, 2025 22:04
Comment on lines +3617 to +3587
otherwise, completed successfully. In all cases, any number of elements (from
`0` to `n`) may have *first* been copied into or out of the buffer passed to
the `read` or `write` and so this number is packed into the `i32` result.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to provide a guarantee that if CANCELLED is returned then the number of elements transferred is always 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that it was indeed possible to receive CANCELLED and >0 elements. Hypothetically this could occur even with component-to-component streaming if a few partial writes succeed but then, when the reader tries to cancel, the writer is (in a hypothetical future with threads) on a different thread and the runtime is holding a lock while doing a large copy between buffers, and maybe we allow in this case the cancellation to return BLOCKED, and then it seems like we'd expect CANCELLED with >0 elements (as opposed to COMPLETED; the idea being that COMPLETED means "on its own", which is perhaps a distinction without a difference; I dunno?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense to me, and I don't think this should be too too hard to bind in various languages. There's going to be some subtelty to interpreting the return values but I don't think that'll be too bad.

@alexcrichton
Copy link
Collaborator

Originally, I was going to put the 2-bit "result" field in the MSBs such

I was sort of having second thoughts about this too so I think lining things up seems reasonable. As you say getting multi-value means that we can lift all these restrictions anyway

@lukewagner lukewagner merged commit 4fb85b4 into main Apr 14, 2025
2 checks passed
@lukewagner lukewagner deleted the copy-result branch April 14, 2025 22:37
alexcrichton added a commit to alexcrichton/wit-bindgen that referenced this pull request Apr 15, 2025
alexcrichton added a commit to alexcrichton/wasip3-prototyping that referenced this pull request Apr 15, 2025
github-merge-queue bot pushed a commit to bytecodealliance/wit-bindgen that referenced this pull request Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants