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

Align WritableStream structure with ReadableStream structure #488

Closed
wants to merge 19 commits into
base: master
from

Conversation

2 participants
@tyoshino
Member

tyoshino commented Aug 1, 2016

  • Add WritableStreamDefaultWriter
  • Add WritableStreamDefaultController
  • Add lock related methods
  • Update reader.pipeTo() to use the new WritableStream classes
  • Refine reader.pipeTo()'s error handling
  • Update TransformStream to use the new WritableStream classes
  • Refine TransformStream's error handling
  • Document the difference between ReadableStream's locking (releaseLock()) and WritableStream's one
    • WritableStream's reader's releaseLock() doesn't check pending writes
    • moved to #516
Align WritableStream structure with ReadableStream structure
- Add WritableStreamDefaultWriter
- Add WritableStreamDefaultController
- Add lock related methods
- Update reader.pipeTo() to use the new WritableStream classes
@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Aug 1, 2016

Member

Squashed and rebased version of #462.

Member

tyoshino commented Aug 1, 2016

Squashed and rebased version of #462.

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Aug 1, 2016

Member

I inserted a lot of logs this time for debugging pipeTo(). I'd like to leave some or all of them in the reference impl for debugging in the future. Do you have any opinion about that, Domenic?

Member

tyoshino commented Aug 1, 2016

I inserted a lot of logs this time for debugging pipeTo(). I'd like to leave some or all of them in the reference impl for debugging in the future. Do you have any opinion about that, Domenic?

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Aug 1, 2016

Member

In rewriting pipeTo() algorithm, I felt that it's good if we apply some convention to variable naming to distinguish variables local to the inner functions and ones of pipeTo() like how we're distinguishing internal slots and local variables. I'm tentatively prefixing them with _.

Member

tyoshino commented Aug 1, 2016

In rewriting pipeTo() algorithm, I felt that it's good if we apply some convention to variable naming to distinguish variables local to the inner functions and ones of pipeTo() like how we're distinguishing internal slots and local variables. I'm tentatively prefixing them with _.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 1, 2016

Member

I'd like to leave some or all of them in the reference impl for debugging in the future. Do you have any opinion about that, Domenic?

I think we should at least comment them out when committing to the repo. In the future we could investigate a logging framework that allows us to toggle them on and off with environment variables, like https://www.npmjs.com/package/debug maybe.

Member

domenic commented Aug 1, 2016

I'd like to leave some or all of them in the reference impl for debugging in the future. Do you have any opinion about that, Domenic?

I think we should at least comment them out when committing to the repo. In the future we could investigate a logging framework that allows us to toggle them on and off with environment variables, like https://www.npmjs.com/package/debug maybe.

return WritableStreamDefaultWriterGetDesiredSize(this)
}
get ready() {

This comment has been minimized.

@domenic

domenic Aug 1, 2016

Member

Are you still planning to eventually make this waitForDesiredSize(n) or similar? Per #318 ?

@domenic

domenic Aug 1, 2016

Member

Are you still planning to eventually make this waitForDesiredSize(n) or similar? Per #318 ?

This comment has been minimized.

@tyoshino

tyoshino Aug 2, 2016

Member

Thanks for reminding me. I think it's ok to keep desiredSize getter. waitForDesiredSize(n) or waitForDesiredSizeChange() could be added to give the same power as the pull() signal for underlyingSource to WritableStream users would be good I still think basically. But it doesn't need to be included in this PR, I guess. WDYT?

@tyoshino

tyoshino Aug 2, 2016

Member

Thanks for reminding me. I think it's ok to keep desiredSize getter. waitForDesiredSize(n) or waitForDesiredSizeChange() could be added to give the same power as the pull() signal for underlyingSource to WritableStream users would be good I still think basically. But it doesn't need to be included in this PR, I guess. WDYT?

This comment has been minimized.

@domenic

domenic Aug 2, 2016

Member

What if we changed ready() from a getter to a function, waitForDesiredSize() with no arguments? Then in the future we could upgrade it to allow arguments. But by default it waits for desiredSize >= highWaterMark.

@domenic

domenic Aug 2, 2016

Member

What if we changed ready() from a getter to a function, waitForDesiredSize() with no arguments? Then in the future we could upgrade it to allow arguments. But by default it waits for desiredSize >= highWaterMark.

This comment has been minimized.

@domenic

domenic Aug 2, 2016

Member

It would technically be a normative change as we would stop returning the same promise every time and start returning a new one for each function invocation. But I think that would be OK.

@domenic

domenic Aug 2, 2016

Member

It would technically be a normative change as we would stop returning the same promise every time and start returning a new one for each function invocation. But I think that would be OK.

This comment has been minimized.

@tyoshino

tyoshino Aug 2, 2016

Member

So, waitForDesiredSize() is equivalent to .. waitForDesiredSize(1), and it would fulfill when the desired size becomes equal to or more than 1?

waitForDesiredSizeChange() can be implemented by

const current = writer.desiredSize;
writer.waitForDesiredSize(current + 1).then(...);

assuming there's no legitimate use of non integer values together with this waiting API.

desiredSize >= highWaterMark.

nitpicking: you meant HWM >= queueSize?

@tyoshino

tyoshino Aug 2, 2016

Member

So, waitForDesiredSize() is equivalent to .. waitForDesiredSize(1), and it would fulfill when the desired size becomes equal to or more than 1?

waitForDesiredSizeChange() can be implemented by

const current = writer.desiredSize;
writer.waitForDesiredSize(current + 1).then(...);

assuming there's no legitimate use of non integer values together with this waiting API.

desiredSize >= highWaterMark.

nitpicking: you meant HWM >= queueSize?

This comment has been minimized.

@domenic

domenic Aug 2, 2016

Member

I guess we are somewhat re-debating (1) vs. (2) from #318. I guess I was thinking (2), but (1) is probably best. If we do (1) then I think you are right.

If we do (2) then waitForDesiredSize() is equivalent to waitForDesiredSize(HWM), i.e. queueSize <= 0. It waits for complete draining instead of just dipping below the HWM.

But yeah, let's do (1).

@domenic

domenic Aug 2, 2016

Member

I guess we are somewhat re-debating (1) vs. (2) from #318. I guess I was thinking (2), but (1) is probably best. If we do (1) then I think you are right.

If we do (2) then waitForDesiredSize() is equivalent to waitForDesiredSize(HWM), i.e. queueSize <= 0. It waits for complete draining instead of just dipping below the HWM.

But yeah, let's do (1).

This comment has been minimized.

@tyoshino

tyoshino Aug 2, 2016

Member

Oops! I see. I wrote the comment without remembering that the point is not yet finalized in #318.

@tyoshino

tyoshino Aug 2, 2016

Member

Oops! I see. I wrote the comment without remembering that the point is not yet finalized in #318.

tyoshino added some commits Aug 1, 2016

Rename promise helpers
Addresses comment #488 (comment)
return undefined;
WritableStreamFulfillWriteRequest(stream);

This comment has been minimized.

@domenic

domenic Aug 1, 2016

Member

Why not fulfill the write request normally? Why is the last write request special?

@domenic

domenic Aug 1, 2016

Member

Why not fulfill the write request normally? Why is the last write request special?

This comment has been minimized.

@tyoshino

tyoshino Aug 1, 2016

Member

_writeRequests now holds both pending write and pending close. That's why we're calling this here.

@tyoshino

tyoshino Aug 1, 2016

Member

_writeRequests now holds both pending write and pending close. That's why we're calling this here.

const ts = new TransformStream({
transform(chunk, enqueue, done) {
transform(chunk, done, enqueue) {

This comment has been minimized.

@domenic

domenic Aug 1, 2016

Member

I don't mind, since I think we will work on transform streams more later, but why change the order of these arguments?

@domenic

domenic Aug 1, 2016

Member

I don't mind, since I think we will work on transform streams more later, but why change the order of these arguments?

This comment has been minimized.

@tyoshino

tyoshino Aug 1, 2016

Member

Yeah, this is what I should clearly explain. I thought that like streams' controller, it might be good to pass the TransformStream controlling functions at the time of creation so that we allow the implementation to start writing some header that is independent from what will be written to the writable side. Then, we'll introduce start(). start() would take function to enqueue(), close() and error(). They don't need to be given to each of transform() and flush() but passed just as a shortcut. But, chunk and done() are specific to each transform() call. So, I thought it would be good to regroup them like this.

@tyoshino

tyoshino Aug 1, 2016

Member

Yeah, this is what I should clearly explain. I thought that like streams' controller, it might be good to pass the TransformStream controlling functions at the time of creation so that we allow the implementation to start writing some header that is independent from what will be written to the writable side. Then, we'll introduce start(). start() would take function to enqueue(), close() and error(). They don't need to be given to each of transform() and flush() but passed just as a shortcut. But, chunk and done() are specific to each transform() call. So, I thought it would be good to regroup them like this.

This comment has been minimized.

@domenic

domenic Aug 1, 2016

Member

Oh, very interesting.

I guess I am still hopeful we can get rid of done by using a promise return. But I don't remember where our discussion landed there. We'll figure it out.

@domenic

domenic Aug 1, 2016

Member

Oh, very interesting.

I guess I am still hopeful we can get rid of done by using a promise return. But I don't remember where our discussion landed there. We'll figure it out.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 1, 2016

Member

Review finished :). Seems like you've got most of my comments already. Big ones are waitForDesiredSize (#488 (comment)) and the different states (#488 (comment)).

Member

domenic commented Aug 1, 2016

Review finished :). Seems like you've got most of my comments already. Big ones are waitForDesiredSize (#488 (comment)) and the different states (#488 (comment)).

tyoshino added some commits Aug 1, 2016

tyoshino added a commit that referenced this pull request Aug 3, 2016

domenic added a commit that referenced this pull request Aug 4, 2016

Perform some caching in the reference implementation
In general we are loathe to introduce this kind of departure from the spec. However, it improves our ability to run the tests quickly by a very large factor. See the context at #488 (comment).
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 4, 2016

Member

I think this is ready! What about you?

Member

domenic commented Aug 4, 2016

I think this is ready! What about you?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Aug 4, 2016

Member

Draft commit message:

Add locking and precise flow control to WritableStream

This fixes #319 (by adding locking and the concept of a writable stream writer) and fixes #318 (by adding precise flow control via the writer.desiredSize property). The structure parallels that introduced for ReadableStream, including the introduction of a controller class.

The infrastructure is set up to allow the future introduction of different types of writable streams (with corresponding new controllers and writers).

With this in place, the public API for writable streams is almost stable; the remaining issue is whether writers should have a ready promise, or a waitForDesiredSize() method which could in the future be customized by passing an argument (see discussions in #318).

The reference implementation's pipeTo and transform stream code has been updated, and all the tests pass, but they haven't been put in the spec yet, as their design is not yet finalized (but is much closer to now that they have a more stable WritableStream foundation).

Merge at will!

Member

domenic commented Aug 4, 2016

Draft commit message:

Add locking and precise flow control to WritableStream

This fixes #319 (by adding locking and the concept of a writable stream writer) and fixes #318 (by adding precise flow control via the writer.desiredSize property). The structure parallels that introduced for ReadableStream, including the introduction of a controller class.

The infrastructure is set up to allow the future introduction of different types of writable streams (with corresponding new controllers and writers).

With this in place, the public API for writable streams is almost stable; the remaining issue is whether writers should have a ready promise, or a waitForDesiredSize() method which could in the future be customized by passing an argument (see discussions in #318).

The reference implementation's pipeTo and transform stream code has been updated, and all the tests pass, but they haven't been put in the spec yet, as their design is not yet finalized (but is much closer to now that they have a more stable WritableStream foundation).

Merge at will!

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Aug 4, 2016

Member

Merged as 5a27534

Member

tyoshino commented Aug 4, 2016

Merged as 5a27534

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Aug 4, 2016

Member

We may want to create ReadableStreamDefaultControllerErrorIfNeeded so that readable and writable are more parallel. I noticed the spec always does a state check for readable streams in the same way.

Done by 28c20b1

Member

tyoshino commented Aug 4, 2016

We may want to create ReadableStreamDefaultControllerErrorIfNeeded so that readable and writable are more parallel. I noticed the spec always does a state check for readable streams in the same way.

Done by 28c20b1

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