error icon indicating copy to clipboard operation
error copied to clipboard

stream error

Open dead-horse opened this issue 11 years ago • 8 comments

we only do try catch here to catch the error. but all the streams' and events' errors are handling by stream.on('error', this.onerror), we can do nothing for them, except hack ctx.onerror method.

should we add an easy way for users to write their own error handler in koa, just like what connect/express do:

app.use(function (err, req, res, next){});

or just leave these to error middleware with hack ctx.onerror?

dead-horse avatar Apr 16 '14 12:04 dead-horse

thought about creating a util to yield until the first readable or error event, but i'm not sure if it's worth it

jonathanong avatar Apr 16 '14 14:04 jonathanong

seems we treat these tow kind of errors in different way

dead-horse avatar Apr 16 '14 14:04 dead-horse

yeah definitely sucks :( I have mixed feelings about adding a root level error handler, it's sort of necessary in this case but then it deviates from the norm, and the fact that you can have multiple error handlers that propagate. streams just don't fit the model really, which is why rails also bypasses middleware when streaming

tj avatar Apr 17 '14 03:04 tj

i try to hacked ctx.onerror in koa-onerror, i can not found another way to deal with all errors in one place. but it is really lame.

should we make ctx.onerror more customizable (e.g. add app.errorHandler= called by ctx.onerror), or just let other middlewares to do this lame thing?

dead-horse avatar Apr 17 '14 18:04 dead-horse

I don't want to form too many opinions based on the current stream implementation since they're super broken. We have a few choices for future stuff I guess, people can stream in the middleware itself, errors would propagate like normal, but it bypasses upstream middleware since you're doing all of the read/write operations in the middleware itself. Or like now with most just assign .body to a stream and let koa do its thing, which then fucks up error handling haha. Both cases kind of suck for different reasons but the latter is probably the most common. I guess what we sort of really need is the ability to prepend middleware, conceptually I don't like that but we have a few other cases for it as well

tj avatar Apr 17 '14 18:04 tj

i was thinking of doing something like this in either the respond middleware or a error handler. @dead-horse maybe you want to play with this idea?

var stream = this.body
if (stream.pipe) {
  yield function (_done) {
    stream.read(0)
    stream.on('readable', done)
    stream.on('error', done)
    function done(err) {
      stream.removeListener('readable', done)
      stream.removeListener('error', done)
      _done(err)
    }
  }
}

it just makes sure the first event isn't an error, which would happen with fs streams and stuff. maybe listen to the close event as well - dunno.

only think i can think of is that doing .read(0) would end empty streams, which i consider a stream bug.

jonathanong avatar Apr 17 '14 20:04 jonathanong

i want to make sure all errors can caught by my own, so i can decide how to display it. but stream sucks . :( fortunately in most usage scenarios, streams not necessary. who really want to use it, must understand its disgusting place. maybe we just point this out to let users know haha

dead-horse avatar Apr 18 '14 03:04 dead-horse

streams not necessary. who really want to use it, must understand its disgusting place.

amen!

jonathanong avatar Apr 18 '14 04:04 jonathanong