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

Update TransformStream API & misc. fixups #519

Merged
merged 29 commits into from Oct 12, 2016

Conversation

4 participants
@isonmad
Contributor

isonmad commented Sep 16, 2016

First pass at issue #518 to align with the other stream APIs. Ended up also addressing #498, #331, and #185.

isonmad added some commits Sep 16, 2016

isonmad
fix thrown error in transformstream enqueue
transformStream._error does not exist.

This codepath really needs a test.
isonmad
add TransformStreamDefaultController
Align the TransformStream API to resemble ReadableStream more closely.

Issue #518.
isonmad
Return a Promise from TransformStream transform()
Align with the WritableStream API.

This resolves issues #518, #498, #331, and #185.
isonmad
Make TransformStream transformer.flush() like sink.close()
Return a promise with the same semantics as WritableStream's
underlyingSink.close(). This also resolves an old comment,
as well as issue #518.

Consequently, calling controller.close() after flush() is
called now throws a TypeError.
@ricea

This comment has been minimized.

Show comment
Hide comment
@ricea

ricea Sep 20, 2016

Collaborator

If I understand correctly, this doesn't enforce any ordering guarantees between the start/transform/flush methods. I would like to have the guarantee that transform() is not called until start()'s Promise resolves (and not at all if it rejects), and flush() is not called until transform()'s Promise resolves.

The reason is that if transform() is called before the Promise returned by start() resolves, then a call to enqueue() by transform() could end up happening before before a call to enqueue() by start(). Similarly with transform() and flush().

It's possible to enforce the ordering within the transformer, but that seems likely to cause a steep learning curve as every developer rediscovers the issue.

I can file a separate issue for this if you like.

Collaborator

ricea commented Sep 20, 2016

If I understand correctly, this doesn't enforce any ordering guarantees between the start/transform/flush methods. I would like to have the guarantee that transform() is not called until start()'s Promise resolves (and not at all if it rejects), and flush() is not called until transform()'s Promise resolves.

The reason is that if transform() is called before the Promise returned by start() resolves, then a call to enqueue() by transform() could end up happening before before a call to enqueue() by start(). Similarly with transform() and flush().

It's possible to enforce the ordering within the transformer, but that seems likely to cause a steep learning curve as every developer rediscovers the issue.

I can file a separate issue for this if you like.

@isonmad

This comment has been minimized.

Show comment
Hide comment
@isonmad

isonmad Sep 20, 2016

Contributor

I would like to have the guarantee that transform() is not called until start()'s Promise resolves (and not at all if it rejects), and flush() is not called until transform()'s Promise resolves.

start() doesn't return a promise yet, but adding one with that invariant makes sense. But flush() is only called from sink.close(), and that's already guaranteed not to be called until the last sink.write() promise resolves, which is only resolved when a transform() promise resolves, if I understand it all correctly. There's already an assert(transformStream._transforming === false); before the flush() call to that effect.

Contributor

isonmad commented Sep 20, 2016

I would like to have the guarantee that transform() is not called until start()'s Promise resolves (and not at all if it rejects), and flush() is not called until transform()'s Promise resolves.

start() doesn't return a promise yet, but adding one with that invariant makes sense. But flush() is only called from sink.close(), and that's already guaranteed not to be called until the last sink.write() promise resolves, which is only resolved when a transform() promise resolves, if I understand it all correctly. There's already an assert(transformStream._transforming === false); before the flush() call to that effect.

@ricea

This comment has been minimized.

Show comment
Hide comment
@ricea

ricea Sep 21, 2016

Collaborator

It's not obvious to me why this is not okay. Would it be possible to change the test description to describe the correctness invariant that is being enforced?

Collaborator

ricea commented on reference-implementation/test/transform-stream.js in cad1cc1 Sep 21, 2016

It's not obvious to me why this is not okay. Would it be possible to change the test description to describe the correctness invariant that is being enforced?

isonmad added some commits Sep 22, 2016

isonmad
TransformStream: call transformer.start synchronously
Disregard previous commit. sink.start and source.start are
called synchronously so this should too. Much simpler.
isonmad
remove weird conditional throw in TransformStream constructor
The conditional is always true, so this always throws.

Is there a point in altering the state of an object when
throwing an exception from its constructor anyway?
isonmad
test synchronous transformer.start failure
This matches behavior for readable and writable streams.
isonmad
drop superfluous try-catch in TransformStream enqueue
Per the spec, ReadableStreamDefaultControllerGetDesiredSize
is nothrow. Nothing can go wrong with just adding up numbers.
isonmad
TransformStream: handle readableStrategy.size() errors
Before, it was trying to error the readable end, when it
had already errored itself.
@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Sep 29, 2016

Member

Sorry for delay. I'll review this soon.

Member

tyoshino commented Sep 29, 2016

Sorry for delay. I'll review this soon.

@tyoshino

lgtm once the 2 comments are addressed. thanks so much.

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Oct 5, 2016

Member

lgtm. thanks!

Member

tyoshino commented Oct 5, 2016

lgtm. thanks!

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Oct 5, 2016

Member

I'd like to add your name to the acknowledgement section of the spec. Do you want to use isonmad or some other name?

I'm going to squash the commits and merge it. If you want to write the commit summary for the squashed commit, please let me know. I can also write it for you if you want.

Member

tyoshino commented Oct 5, 2016

I'd like to add your name to the acknowledgement section of the spec. Do you want to use isonmad or some other name?

I'm going to squash the commits and merge it. If you want to write the commit summary for the squashed commit, please let me know. I can also write it for you if you want.

isonmad added some commits Oct 10, 2016

isonmad
fixup comments
This function is just one more abstract operation now
and doesn't need its own separate category.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 10, 2016

Member

Just wanted to say thank you @isonmad for all this work! It's really excellent to get this kind of contribution.

I guess we can merge this now with "isonmad" in the acknowledgments, but do feel free to let us know what would be better!

Member

domenic commented Oct 10, 2016

Just wanted to say thank you @isonmad for all this work! It's really excellent to get this kind of contribution.

I guess we can merge this now with "isonmad" in the acknowledgments, but do feel free to let us know what would be better!

@isonmad

This comment has been minimized.

Show comment
Hide comment
@isonmad

isonmad Oct 10, 2016

Contributor

That works fine. If it has to all be one monolithic commit, I guess the commit message would be

move TransformStream to promises based api (#519)

Fix a bunch of bugs along the way and add tests for them.

Closes #518, #498, #331, and #185.
Contributor

isonmad commented Oct 10, 2016

That works fine. If it has to all be one monolithic commit, I guess the commit message would be

move TransformStream to promises based api (#519)

Fix a bunch of bugs along the way and add tests for them.

Closes #518, #498, #331, and #185.

@tyoshino tyoshino merged commit a957cc5 into whatwg:master Oct 12, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Oct 12, 2016

Member

Thanks so much for your work, @isonmad. I've added your name to the acknowledgement section in 294d720

I'm going to make some follow up commits.

Member

tyoshino commented Oct 12, 2016

Thanks so much for your work, @isonmad. I've added your name to the acknowledgement section in 294d720

I'm going to make some follow up commits.

tyoshino added a commit that referenced this pull request Oct 12, 2016

No need to reject writePromise in TransformStreamErrorInternal
Follow up for #519

As we error the writable stream in TransformStreamErrorInternal, we don't need to take care of the promise returned from the underlying sink write().
transformStream._chunkPending = true;
transformStream._chunk = chunk;
const promise = new Promise(resolve => {
transformStream._resolveWrite = resolve;
transformStream._rejectWrite = resolve;

This comment has been minimized.

@tyoshino

tyoshino Oct 12, 2016

Member

this should be set to the rejection callback of the promise

@tyoshino

tyoshino Oct 12, 2016

Member

this should be set to the rejection callback of the promise

if (transformStream._resolveWriter !== undefined) {
transformStream._resolveWriter(undefined);
if (transformStream._rejectWriter !== undefined) {
transformStream._rejectWriter(e);

This comment has been minimized.

@tyoshino

tyoshino Oct 12, 2016

Member

s/rejectWriter/rejectWrite/

anyway, I just noticed that this is unnecessary since we call _writableController.error() at L118. I'm going to just remove this in #533

@tyoshino

tyoshino Oct 12, 2016

Member

s/rejectWriter/rejectWrite/

anyway, I just noticed that this is unnecessary since we call _writableController.error() at L118. I'm going to just remove this in #533

tyoshino added a commit that referenced this pull request Oct 12, 2016

No need to reject writePromise in TransformStreamErrorInternal (#533)
Follow up for #519

As we error the writable stream in TransformStreamErrorInternal, we don't need to take care of the promise returned from the underlying sink write().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment