search

poelstra / ts-stream

Type-safe object streams with seamless support for backpressure, ending, and error handling
MIT License
65 stars 13 forks source link

Stream#from() does not end the stream on abort() #16

Closed rogierschouten closed 9 years ago

rogierschouten commented 9 years ago

It looks like you intended to finish this still, as there is a TODO in the code. Just in case, I'll submit this issue to track it.

Stream.from() does not end on abort, and hence, it also doesn't end on a write error.

poelstra commented 9 years ago

You're correct, it doesn't work as expected after abort behaviour was changed recently. And from() isn't unit-tested yet either.

Will fix after holiday, thanks for reporting :)

On 30 juni 2015 14:16:43 GMT+07:00, Rogier Schouten notifications@github.com wrote:

It looks like you intended to finish this still, as there is a TODO in the code. Just in case, I'll submit this issue to track it.

Stream.from() does not end on abort, and hence, it also doesn't end on a write error.

Reply to this email directly or view it on GitHub: https://github.com/poelstra/ts-stream/issues/16

rogierschouten commented 9 years ago

have fun :)

rikkertkoppes commented 9 years ago

Out of curiosity: how should that work? I see that end() returns a promise, but abort() does not. It seems that the forEach() abort handler should be allowed to return a promise, which is then returned by the abort() method itself. This would signal that the abort process is finished (or error-ed)

Then, from() could be modified to perform stream.abort(error).then(stream.end)

rogierschouten commented 9 years ago

How I've used abort/cancel-type logic so far in any program is to destroy any in-use resources like sockets (which is synchronous), after which the regular work fails in the way it would normally fail for other errors - in this case with a promise rejection. As such, abort() hardly ever needs a promise - if it did, you'd have TWO promises that the work is ended and you'd need to duplicate your handling of that.

The idea is that abort() simply communicates a desire to stop, and it is up to the stream blocks to actually do so - faster than otherwise or not, it's a race condition after all.

As the normal end() handling already has to account for failures, it is not necessary to invent another mechanism for abort(). Also, what would a rejection on an abort() call mean? Did it stop or not?

I.e. you do

stream.result()
  .then(...)
  .catch(...)

// oh no we need to abort!
stream.abort(new AbortError("I just felt like aborting today"));

In our code we use special AbortErrors to signal to others that stopping was intentional.

rogierschouten commented 9 years ago

@rikkertkoppes I just noticed I hadn't fully answered your question - how would from() work?

It used to be that abort() caused all writes to fail, prompting the from() to end() the stream prematurely. However, as you can see in Transform.js, streams now also have an aborted() method that does return a promise. I wrote some of this code already, am now not in a position to look it up. Maybe @poelstra can tell us off the top of his head when he's back :)

poelstra commented 9 years ago

Fixed in 0.6.1.

poelstra commented 9 years ago

Note: from() currently aborts the stream when an error occurs (and then properly ends the stream, too).

I'm a bit unsure whether just calling end(someError) (and not aborting) may be a better idea instead. Problem I foresee is that people may start to expect that streams are 'always'/supposed to be aborted when an error occurs, based on their playing around with from()-based examples, which is not necessarily the case.

rogierschouten commented 9 years ago

It's a design choice, and any choice you make will not fit half the use cases.

This also depends on #22. If a write error prevents subsequent writes, then an end() would be fine, otherwise you'd want the abort behavior by default to avoid surprising anyone.

poelstra commented 9 years ago

Exactly, and as per https://github.com/poelstra/ts-stream/issues/22#issuecomment-124016355, a write error does not prevent subsequent writes by default, in which 'just' an end() might make more sense.

I think I'm going to create some more examples in order to get a better feel for what fits best. If you have use cases that may require one or the other, please let me know :)

rogierschouten commented 9 years ago

I think when you're using from(), it's not expected that a write gives an error. After all, one cannot expect the from() source to handle errors in a meaningful way. Therefore, I would not expect another write(), as this would potentially cause the original error to be obscured by a follow-up error (I've had this in the past, where an app continued for a while after a programming error)

poelstra commented 9 years ago

Agreed. And indeed from() itself stops after the first error. The 'note' I mentioned before was about whether from() should call abort() in that case (in addition to calling end(theWriteError)).

Having it abort makes sure that the error shows up in result() as soon as possible, but as said, it may trick people into thinking that streams will always abort on error (because most of them will start playing with from() in toy examples and unit tests).