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

pipeTo with preventCancel: true never settle if readable doesn't produce new chunks #118

Closed
yume-chan opened this issue May 21, 2022 · 2 comments
Labels
bug domain: compliance Issues related to compliance with the streams standard
Milestone

Comments

@yume-chan
Copy link

Repro

const readable = new ReadableStream();
const abortController = new AbortController();

readable.pipeTo(new WritableStream(), {
    preventCancel: true,
    signal: abortController.signal,
}).then(() => {
    console.log('resolve');
}, () => {
    console.log('rejected');
});

setTimeout(() => {
    console.log('abort');
    abortController.abort();
}, 1000);

setTimeout(() => {
    console.log('exit');
}, 10000);

Expected

Microsoft Edge canary 103.0.1255.0, with native ReadableStream and WritableStream

abort
rejected
exit

Node.js v16.13.1, with stream/web

(node:14660) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
abort
rejected
exit

Actual

web-streams-polyfill: 4.0.0-beta.2

abort
exit
@MattiasBuelens
Copy link
Owner

MattiasBuelens commented May 22, 2022

Good catch! Indeed, the next version of the polyfill seems to deviate from the specification. 🤔

The polyfill waits for both reads and writes to finish before shutting down the pipe. However, the reference implementation and most browser implementations only wait until all writes have finished. Any read requests that have been started by the pipe but haven't finished yet are "silently" discarded.

I think I did this because we had to ensure there were no more pending read requests before the shutdown process calls reader.releaseLock() (see d77b1dd). However, that was before we changed releaseLock()'s behavior in whatwg/streams#1168. So really, the polyfill no longer needs to (and actually shouldn't) do this anymore.

I'll look into it. 🕵️

@MattiasBuelens MattiasBuelens added bug domain: compliance Issues related to compliance with the streams standard labels May 22, 2022
@MattiasBuelens MattiasBuelens added this to the v4.0.0-beta.3 milestone May 22, 2022
@MattiasBuelens
Copy link
Owner

Fixed in v4.0.0-beta.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug domain: compliance Issues related to compliance with the streams standard
Projects
None yet
Development

No branches or pull requests

2 participants