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

Have internal streams return empty Uint8Array on end of byob stream #2045

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 22, 2024

Per the spec, a BYOB read is supposed to return an empty ArrayBufferView when the stream is closed (done = true) in order to allow the code reading to retake ownership of the underlying buffer. This change makes the internal streams implementation return an empty Uint8Array in this case, matching the behavior of the JS-backed standard streams impl.

It's unlikely but this may be considered a breaking change if anyone has code that depends on the fact that value would be undefined if done === true when using the old streams implementation. We need to decide if this case needs a compat flag or not. I'd prefer not to have to introduce Yet Another One. Updated to add the compat flag :-(

@jasnell jasnell requested review from a team as code owners April 22, 2024 18:17
@kentonv
Copy link
Member

kentonv commented Apr 22, 2024

Unfortunately, I could pretty easily imagine someone checking the truthiness of value instead of checking for done.

@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2024

Hmm... all of the documentated examples pretty much everywhere show checking the done = true as the correct way to determine completion, especially given that in a non-BYOB read, value: undefined (or value: false, etc) is a perfectly valid read result while the stream is still active. Unfortunately, however, we don't really have any way of know if someone is checking the truthiness of value instead of doing it the right way. :-( ... So yeah... sadly this will probably require a compat flag and that makes me very sad.

@jasnell jasnell force-pushed the jsnell/internal-stream-empty-result branch from 5b2e336 to 81b504f Compare April 22, 2024 19:16
@jasnell jasnell requested a review from kentonv April 22, 2024 19:17
@jasnell jasnell added the api label Apr 22, 2024
@kentonv
Copy link
Member

kentonv commented Apr 25, 2024

I feel like I personally have written code which checked the truthiness, and it worked. Some time later I think I did realize it should check done instead. But it would have been easy for me to not have noticed the problem -- so I'm sure someone out there has gotten it "wrong". ("Wrong" in quotes because Hyrum's Law says it's not wrong.)

Per the spec, a BYOB read is supposed to return an empty ArrayBufferView
when the stream is closed (done = true) in order to allow the code reading
to retake ownership of the underlying buffer. This change makes the internal
streams implementation return an empty Uint8Array in this case, matching
the behavior of the JS-backed standard streams impl.
@jasnell jasnell force-pushed the jsnell/internal-stream-empty-result branch from 81b504f to 6b49857 Compare April 26, 2024 14:19
@jasnell jasnell merged commit 4e282ed into main Apr 26, 2024
10 checks passed
@jasnell jasnell deleted the jsnell/internal-stream-empty-result branch April 26, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants