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.

Sign up for GitHub

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

Streams: tests for Transformer.cancel #40453

Merged
merged 7 commits into from
Sep 29, 2023
Merged

Streams: tests for Transformer.cancel #40453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff3c698
Streams: tests for `Transformer.cancel`
lucacasonato Jun 8, 2023
4156716
Version 2: run cancel before cancelling
lucacasonato Jun 12, 2023
1f2f6c8
Fixes
lucacasonato Jul 17, 2023
1fbae6a
Add semicolons
MattiasBuelens Jul 17, 2023
9fa9a0c
Add shadowrealm
domenic Sep 29, 2023
ab3f840
lint
lucacasonato Sep 29, 2023
a169eb4
Merge remote-tracking branch 'origin/master' into TransformStream_cancel
lucacasonato Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions streams/transform-streams/cancel.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// META: global=window,worker,shadowrealm
// META: script=../resources/test-utils.js
'use strict';

const thrownError = new Error('bad things are happening!');
thrownError.name = 'error1';

const originalReason = new Error('original reason');
originalReason.name = 'error2';

promise_test(async t => {
let cancelled = undefined;
const ts = new TransformStream({
cancel(reason) {
cancelled = reason;
}
});
const res = await ts.readable.cancel(thrownError);
assert_equals(res, undefined, 'readable.cancel() should return undefined');
assert_equals(cancelled, thrownError, 'transformer.cancel() should be called with the passed reason');
}, 'cancelling the readable side should call transformer.cancel()');

promise_test(async t => {
const ts = new TransformStream({
cancel(reason) {
assert_equals(reason, originalReason, 'transformer.cancel() should be called with the passed reason');
throw thrownError;
}
});
const writer = ts.writable.getWriter();
const cancelPromise = ts.readable.cancel(originalReason);
await promise_rejects_exactly(t, thrownError, cancelPromise, 'readable.cancel() should reject with thrownError');
await promise_rejects_exactly(t, thrownError, writer.closed, 'writer.closed should reject with thrownError');
}, 'cancelling the readable side should reject if transformer.cancel() throws');

promise_test(async t => {
let aborted = undefined;
const ts = new TransformStream({
cancel(reason) {
aborted = reason;
},
flush: t.unreached_func('flush should not be called')
});
const res = await ts.writable.abort(thrownError);
assert_equals(res, undefined, 'writable.abort() should return undefined');
assert_equals(aborted, thrownError, 'transformer.abort() should be called with the passed reason');
}, 'aborting the writable side should call transformer.abort()');

promise_test(async t => {
const ts = new TransformStream({
cancel(reason) {
assert_equals(reason, originalReason, 'transformer.cancel() should be called with the passed reason');
throw thrownError;
},
flush: t.unreached_func('flush should not be called')
});
const reader = ts.readable.getReader();
const abortPromise = ts.writable.abort(originalReason);
await promise_rejects_exactly(t, thrownError, abortPromise, 'writable.abort() should reject with thrownError');
await promise_rejects_exactly(t, thrownError, reader.closed, 'reader.closed should reject with thrownError');
}, 'aborting the writable side should reject if transformer.cancel() throws');

promise_test(async t => {
const ts = new TransformStream({
async cancel(reason) {
assert_equals(reason, originalReason, 'transformer.cancel() should be called with the passed reason');
throw thrownError;
},
flush: t.unreached_func('flush should not be called')
});
const cancelPromise = ts.readable.cancel(originalReason);
const closePromise = ts.writable.close();
await Promise.all([
promise_rejects_exactly(t, thrownError, cancelPromise, 'cancelPromise should reject with thrownError'),
promise_rejects_exactly(t, thrownError, closePromise, 'closePromise should reject with thrownError'),
]);
}, 'closing the writable side should reject if a parallel transformer.cancel() throws');

promise_test(async t => {
let controller;
const ts = new TransformStream({
start(c) {
controller = c;
},
async cancel(reason) {
assert_equals(reason, originalReason, 'transformer.cancel() should be called with the passed reason');
controller.error(thrownError);
},
flush: t.unreached_func('flush should not be called')
});
const cancelPromise = ts.readable.cancel(originalReason);
const closePromise = ts.writable.close();
await Promise.all([
promise_rejects_exactly(t, thrownError, cancelPromise, 'cancelPromise should reject with thrownError'),
promise_rejects_exactly(t, thrownError, closePromise, 'closePromise should reject with thrownError'),
]);
}, 'readable.cancel() and a parallel writable.close() should reject if a transformer.cancel() calls controller.error()');

promise_test(async t => {
let controller;
const ts = new TransformStream({
start(c) {
controller = c;
},
async cancel(reason) {
assert_equals(reason, originalReason, 'transformer.cancel() should be called with the passed reason');
controller.error(thrownError);
},
flush: t.unreached_func('flush should not be called')
});
const cancelPromise = ts.writable.abort(originalReason);
await promise_rejects_exactly(t, thrownError, cancelPromise, 'cancelPromise should reject with thrownError');
const closePromise = ts.readable.cancel(1);
await promise_rejects_exactly(t, thrownError, closePromise, 'closePromise should reject with thrownError');
}, 'writable.abort() and readable.cancel() should reject if a transformer.cancel() calls controller.error()');
43 changes: 31 additions & 12 deletions streams/transform-streams/errors.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ promise_test(t => {
const ts = new TransformStream({
transform() {
throw thrownError;
}
},
cancel: t.unreached_func('cancel should not be called')
});

const reader = ts.readable.getReader();
Expand All @@ -34,7 +35,8 @@ promise_test(t => {
},
flush() {
throw thrownError;
}
},
cancel: t.unreached_func('cancel should not be called')
});

const reader = ts.readable.getReader();
Expand All @@ -54,13 +56,14 @@ promise_test(t => {
]);
}, 'TransformStream errors thrown in flush put the writable and readable in an errored state');

test(() => {
test(t => {
new TransformStream({
start(c) {
c.enqueue('a');
c.error(new Error('generic error'));
assert_throws_js(TypeError, () => c.enqueue('b'), 'enqueue() should throw');
}
},
cancel: t.unreached_func('cancel should not be called')
});
}, 'errored TransformStream should not enqueue new chunks');

Expand All @@ -72,7 +75,8 @@ promise_test(t => {
});
},
transform: t.unreached_func('transform should not be called'),
flush: t.unreached_func('flush should not be called')
flush: t.unreached_func('flush should not be called'),
cancel: t.unreached_func('cancel should not be called')
});

const writer = ts.writable.getWriter();
Expand All @@ -96,7 +100,8 @@ promise_test(t => {
});
},
transform: t.unreached_func('transform should never be called if start() fails'),
flush: t.unreached_func('flush should never be called if start() fails')
flush: t.unreached_func('flush should never be called if start() fails'),
cancel: t.unreached_func('cancel should never be called if start() fails')
});

const writer = ts.writable.getWriter();
Expand Down Expand Up @@ -202,9 +207,10 @@ promise_test(t => {
return Promise.all([
abortPromise,
cancelPromise,
promise_rejects_exactly(t, thrownError, writer.closed, 'writer.closed should reject with thrownError')]);
}, 'abort should set the close reason for the writable when it happens before cancel during start, but cancel should ' +
'still succeed');
promise_rejects_exactly(t, thrownError, writer.closed, 'writer.closed should reject'),
]);
}, 'abort should set the close reason for the writable when it happens before cancel during start, and cancel should ' +
'reject');

promise_test(t => {
let resolveTransform;
Expand Down Expand Up @@ -251,13 +257,26 @@ promise_test(t => {
controller = c;
}
});
const cancelPromise = ts.readable.cancel(thrownError);
controller.error(ignoredError);
const cancelPromise = ts.readable.cancel(ignoredError);
controller.error(thrownError);
return Promise.all([
cancelPromise,
promise_rejects_exactly(t, thrownError, ts.writable.getWriter().closed, 'closed should reject with thrownError')
]);
}, 'controller.error() should do nothing after readable.cancel()');
}, 'controller.error() should close writable immediately after readable.cancel()');

promise_test(t => {
let controller;
const ts = new TransformStream({
start(c) {
controller = c;
}
});
return ts.readable.cancel(thrownError).then(() => {
controller.error(ignoredError);
return promise_rejects_exactly(t, thrownError, ts.writable.getWriter().closed, 'closed should reject with thrownError');
});
}, 'controller.error() should do nothing after readable.cancel() resolves');

promise_test(t => {
let controller;
Expand Down
15 changes: 15 additions & 0 deletions streams/transform-streams/flush.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,18 @@ promise_test(t => {
});
return promise_rejects_exactly(t, error1, ts.writable.getWriter().close(), 'close() should reject');
}, 'error() during flush should cause writer.close() to reject');

promise_test(async t => {
let flushed = false;
const ts = new TransformStream({
flush() {
flushed = true;
},
cancel: t.unreached_func('cancel should not be called')
});
const closePromise = ts.writable.close();
await delay(0);
const cancelPromise = ts.readable.cancel(error1);
await Promise.all([closePromise, cancelPromise]);
assert_equals(flushed, true, 'transformer.flush() should be called');
}, 'closing the writable side should call transformer.flush() and a parallel readable.cancel() should not reject');
19 changes: 17 additions & 2 deletions streams/transform-streams/general.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,24 @@ promise_test(t => {
controller.terminate();
return Promise.all([
cancelPromise,
promise_rejects_exactly(t, cancelReason, ts.writable.getWriter().closed, 'closed should reject with cancelReason')
promise_rejects_js(t, TypeError, ts.writable.getWriter().closed, 'closed should reject with TypeError')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see. Since TransformStreamDefaultSourceCancelAlgorithm takes at least one microtask to run transformer.cancel() (even if it does not exist), the TypeError from terminate() now wins the race. 🤔

I suppose that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

hello @MattiasBuelens i am trying to implement this for node this test presently fails my knowledge of WPTs is little limited hence asking here so is cancelPromise also expected to throw in this test? I am seeing that running this test standalone throws TypeError but unable to understand why the WPT says this uncaught

Copy link
Member Author

Choose a reason for hiding this comment

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

This test will fail if you have not implanted the changes from the streams spec PR (yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

This test passes against the reference implementation, so if it fails it may be a bug in your implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a bug no doubt i just wnated to know this test is basically equalt to the following script right:

import { TransformStream } from 'stream/web';

let controller;
const ts = new TransformStream({
  start(c) {
    controller = c;
  }
});
const cancelReason = { name: 'cancelReason' };
const cancelPromise = ts.readable.cancel(cancelReason);
controller.terminate();

try {
  await Promise.all([cancelPromise, ts.writable.getWriter().closed]);
} catch (err) {
  console.log(err); // TypeError!
}

Copy link
Contributor

@MattiasBuelens MattiasBuelens Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@debadree25 Yes, that looks script right. Ideally, you want to assert that the TypeError originates from .closed and not from cancelPromise, so in Node you could use assert.rejects():

import assert from 'node:assert/strict';

await Promise.all([
  cancelPromise,
  assert.rejects(ts.writable.getWriter().closed, TypeError)
]);

so is cancelPromise also expected to throw in this test?

No, cancelPromise should always resolve.

The only change is that writer.closed now becomes errored by controller.terminate(), whereas previously it would be errored by ts.readable.cancel(cancelReason). This is because readable.cancel() now needs to go through the (possibly asynchronous) transformer.cancel() method first before it can error the writable, whereas controller.terminate() always errors the writable synchronously (without going through a transformer method).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much @MattiasBuelens for the detail response i guess i have found the issue with my implementation!

]);
}, 'terminate() should do nothing after readable.cancel()');
}, 'terminate() should abort writable immediately after readable.cancel()');

promise_test(t => {
let controller;
const ts = new TransformStream({
start(c) {
controller = c;
}
});
const cancelReason = { name: 'cancelReason' };
return ts.readable.cancel(cancelReason).then(() => {
controller.terminate();
return promise_rejects_exactly(t, cancelReason, ts.writable.getWriter().closed, 'closed should reject with TypeError');
})
}, 'terminate() should do nothing after readable.cancel() resolves');


promise_test(() => {
let calls = 0;
Expand Down
4 changes: 4 additions & 0 deletions streams/transform-streams/reentrant-strategies.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,10 @@ promise_test(t => {
// call to TransformStreamDefaultSink.
return delay(0).then(() => {
controller.enqueue('a');
return reader.read();
}).then(({ value, done }) => {
assert_false(done, 'done should be false');
assert_equals(value, 'a', 'value should be correct');
return Promise.all([promise_rejects_exactly(t, error1, reader.read(), 'read() should reject'), abortPromise]);
});
}, 'writer.abort() inside size() should work');