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

Execute strategySize first in write() #680

Merged
merged 1 commit into from Feb 17, 2017

Conversation

3 participants
@ricea
Collaborator

ricea commented Feb 10, 2017

A strategy size() function can re-entrantly call controller, writer or writable
methods, leaving the stream in an unexpected state. To ensure predictable
behaviour, call size() before observing or modifying any state.

Previously, if size() threw an exception, writer.write() would reject with that
exception. After this change, it rejects with a TypeError instead.

Fixes #675.

@domenic

Some questions about the code/spec changes. Tests look great, including the comment block.

@@ -505,16 +505,6 @@ class WritableStreamDefaultWriter {
return Promise.reject(defaultWriterBrandCheckException('write'));
}
const stream = this._ownerWritableStream;

This comment has been minimized.

@domenic

domenic Feb 10, 2017

Member

I am sad about moving these checks as I like the separation of invariant-checking in the public API only. It seems like at least the first check, of stream being undefined, can stay here. What about the second one?

@domenic

domenic Feb 10, 2017

Member

I am sad about moving these checks as I like the separation of invariant-checking in the public API only. It seems like at least the first check, of stream being undefined, can stay here. What about the second one?

This comment has been minimized.

@ricea

ricea Feb 13, 2017

Collaborator

I moved the stream check back into write().

We need to check state after strategy.size() returns, since it may have changed. We could additionally check it before, if we wanted to guarantee that size() is only called in the writable state, but there are no tests for that and I didn't consider it important.

I have started thinking of size() as part of the public API. This makes the line between public methods and abstract operations a bit fuzzier.

@ricea

ricea Feb 13, 2017

Collaborator

I moved the stream check back into write().

We need to check state after strategy.size() returns, since it may have changed. We could additionally check it before, if we wanted to guarantee that size() is only called in the writable state, but there are no tests for that and I didn't consider it important.

I have started thinking of size() as part of the public API. This makes the line between public methods and abstract operations a bit fuzzier.

@@ -747,6 +747,21 @@ function WritableStreamDefaultControllerClose(controller) {
WritableStreamDefaultControllerAdvanceQueueIfNeeded(controller);
}
function WritableStreamDefaultControllerGetChunkSize(controller, chunk) {

This comment has been minimized.

@domenic

domenic Feb 10, 2017

Member

This appears to be called from only one place, so inlining it fits better with our style.

@domenic

domenic Feb 10, 2017

Member

This appears to be called from only one place, so inlining it fits better with our style.

This comment has been minimized.

@ricea

ricea Feb 13, 2017

Collaborator

Originally I didn't inline it because it was a controller operation rather than a writer operation.

Afterwards I realised that once we have a ByteController this will become a polymorphic method call. Maybe the "you ain't gonna need it" principle applies, but I'm pretty sure that we will add a byte controller and that this separation will make sense at that time.

@ricea

ricea Feb 13, 2017

Collaborator

Originally I didn't inline it because it was a controller operation rather than a writer operation.

Afterwards I realised that once we have a ByteController this will become a polymorphic method call. Maybe the "you ain't gonna need it" principle applies, but I'm pretty sure that we will add a byte controller and that this separation will make sense at that time.

@domenic

LGTM with nit

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 14, 2017

Member

Spec changes LGTM too. Will let you coordinate landing WPTs and then this.

Member

domenic commented Feb 14, 2017

Spec changes LGTM too. Will let you coordinate landing WPTs and then this.

@tyoshino

lgtm

@@ -656,20 +650,26 @@ function WritableStreamDefaultWriterWrite(writer, chunk) {
assert(stream !== undefined);
const controller = stream._writableStreamController;
const chunkSize = WritableStreamDefaultControllerGetChunkSize(controller, chunk);

This comment has been minimized.

@tyoshino

tyoshino Feb 16, 2017

Member

It's really unfortunate that we need to do this in the Writer's abstract op. Ideally, the detail of the operation should be hidden. But ok for now.

@tyoshino

tyoshino Feb 16, 2017

Member

It's really unfortunate that we need to do this in the Writer's abstract op. Ideally, the detail of the operation should be hidden. But ok for now.

@ricea ricea referenced this pull request Feb 17, 2017

Open

Chunking with Streams #171

Execute strategySize first in write()
A strategy size() function can re-entrantly call controller, writer or writable
methods, leaving the stream in an unexpected state. To ensure predictable
behaviour, call size() before observing or modifying any state.

Previously, if size() threw an exception, writer.write() would reject with that
exception. After this change, it rejects with a TypeError instead.

Also update the web-platform-test to include the new
streams/writable-streams/reentrant-strategy.js tests.

@ricea ricea merged commit 311b52c into whatwg:master Feb 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ricea ricea deleted the ricea:fix-reentrant-size branch Feb 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment