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

Copy bytes from BufferSource in parallel #165

Open
reillyeon opened this issue Jul 7, 2022 · 3 comments
Open

Copy bytes from BufferSource in parallel #165

reillyeon opened this issue Jul 7, 2022 · 3 comments

Comments

@reillyeon
Copy link
Collaborator

The write algorithm copies the contents of the chunk before entering the "in parallel" steps. This doesn't match the behavior in Chromium which copies the chunk piecewise into an internal ring buffer as space becomes available. This means that if script modifies the contents of the BufferSource before the Promise returned by write() resolves the data written to the port will change. I think this behavior is correct but should be explicitly specified. The simplest change would be to move the invocation of the "get a copy of the buffer source" algorithm to after the steps go "in parallel". A more accurate description would be to acknowledge that a write may partially complete and so the steps must retry with unwritten pieces of the chunk. WebIDL doesn't seem to define an algorithm for converting a range within a BufferSource to a byte sequence.

CC @domenic

@domenic
Copy link
Collaborator

domenic commented Jul 8, 2022

Hmm. This is generally something we try to avoid in web API design, by either making copies or taking ownership of the entire buffer via transferring. See https://w3ctag.github.io/design-principles/#js-rtc . (At least, I assume what you're describing would violate run to completion, since within a single event loop task, the other thread might first read one version of the buffer, then read another after the JS has updated it.)

Since we don't yet have writable byte streams with ownership taking/returning semantics, we've been assuming people would make copies...

@reillyeon
Copy link
Collaborator Author

Outside the web platform it is a pretty common contract for asynchronous I/O functions to receive a buffer and expect the caller not to mutate that buffer until the operation is complete. I agree that copying or transferring the buffer on write() is safer but has the disadvantage of either performing extra copies or not allowing the caller to reuse the buffer. The current design of the write algorithm sounds like it is implementing a version what you are intending to explicitly support with ownership taking/returning semantics.

The documentation of the underlying sink API should probably include a note about the expectation that the chunk is copied if write() cannot complete synchronously.

@reillyeon
Copy link
Collaborator Author

Following up here because I want to make sure to provide the right guidance on this for other UnderlyingSink implementers. I'm okay with changing the behavior of the Web Serial API in Chromium if the intention is for Streams to eventually support explicitly transfering the buffer to the UnderlyingSink and later returning it to the caller. It looks like this is being discussed in whatwg/streams#495 and the summary mentions the current ambiguities.

domenic added a commit to whatwg/streams that referenced this issue Aug 9, 2022
domenic added a commit to whatwg/streams that referenced this issue Aug 9, 2022
domenic added a commit to whatwg/streams that referenced this issue Sep 6, 2022
See WICG/serial#165. This ends up adding two pieces of advice:

* One about how producers should wait on the promise returned by writer.write(), and how underlying sinks and transformers can use their promise return values to control that.

* One about how specification authors should avoid reading from chunks in parallel.
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

No branches or pull requests

2 participants