alert underlyingSink when [[strategySize]]() fails

[isonmad]

trafficstars

Like the code currently notes:

// TODO: Should we notify the sink of this error?

For readable streams, the underlyingSource gets notified because the exception gets rethrown from controller.enqueue(), which isn't an option for writable streams.

Without a way to know when the stream becomes errored due to the strategy throwing or returning an invalid number, there's no way to know that it should clean up its resources, or to propagate the error forward if the writable stream is just one end of a transform stream.

Should underlyingSink.abort be overloaded to also be called with a) the exception size() threw, or b) a RangeError if it returned a bad value, and not just when a producer calls writer.abort()? That or add an entirely new callback method just for this to either the sink or to the strategy itself.

[ricea]

My preference is to punt on this issue until we add a finally() method. In the meantime I think we can say that as with write(), handling errors in strategySize is the responsibility of the strategy author.

2016/12/13 午前2:59 "isonmad" [email protected]:

Like the code currently notes:

// TODO: Should we notify the sink of this error?

For readable streams, the underlyingSource gets notified because the exception gets rethrown from controller.enqueue(), which isn't an option for writable streams.

Without a way to know when the stream becomes errored due to the strategy throwing or returning an invalid number, there's no way to know that it should clean up its resources, or to propagate the error forward if the writable stream is just one end of a transform stream.

Should underlyingSink.abort be overloaded to also be called with a) the exception size() threw, or b) a RangeError if it returned a bad value, and not just when a producer calls writer.abort()? That or add an entirely new callback method just for this to either the sink or to the strategy itself.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/whatwg/streams/issues/628, or mute the thread https://github.com/notifications/unsubscribe-auth/AGXnh_YNeAn35q0kltMZm6AG6zETx4_Lks5rHgodgaJpZM4LLSOg .

[isonmad]

It's worth noting that for TransformStreams, the strategy author and the sink author aren't the same.

Even if you try to wrap strategy.size() inside another size() that does try-catch and rethrows the error, it, unlike with write(), also has to duplicate the error checking in EnqueueValueWithSize, in which case the RangeError it produces on its own won't be equal to the RangeError thrown by EnqueueValueWithSize.

[domenic]

I think the exception is being propagated to the right place already: to the caller of write(), which caused this to happen. (Even if it isn't their fault.) I think that leaves us in the same position as we are for several other errors, which is that it's currently not easy for the underlying source to clean up after them; that's the idea behind adding a finally() type construct.

[isonmad]

I think the exception is being propagated to the right place already

If an error occurs in a transformstream that's part of a pipe chain, is the error not supposed to be propagated both backwards and forwards?

Like I said in the OP, this makes propagating the error forward down the pipe chain from a transformstream impossible, in the current state of things.

const rs = getReadableStream();
const transformed = rs.pipeThrough(new TransformStream({}, new StrategyThatThrows()));
const reader = transformed.getReader();

If the intermediate TransformStream's strategy throws, it will propagate backwards and cancel rs, but transformed will just mysteriously hang and never receive any new chunks, because the writable end of the transformstream is errored and not doing anything anymore.

finally would definitely solve it, so should we consider transformstreams blocked on adding finally then?

[domenic]

If an error occurs in a transformstream that's part of a pipe chain, is the error not supposed to be propagated both backwards and forwards?

I see. So this is specifically a transform stream problem, of how to get the error from the writable side to the readable side. (Although the most natural solution would indeed involve notifying the underlying sink inside the general writable stream mechanisms.)

And you're saying that the other instances of an error not being delivered to abort() are not a problem, like https://github.com/whatwg/streams/issues/620, because our underlying sink write() inside transform streams is entirely under our control---whereas the queueing strategy is not.

Off the top of my head I can think of two solutions besides notifying the underlying sink: waiting for finally, and creating some kind of wrapper around writableStrategy. (Are we still planning to have both strategies? :-/.) Maybe there are better ones.

I would feel better if there were other reasons finally was important for transform streams.

As for the idea of notifying the underlying sink, it does not seem correct, at least as currently envisioned. abort() is only called in reaction to the producer actually saying writer.abort() currently, and it should probably stay that way.