Add support for headers in errors

[PlasmaPower]

Resolves #571. I'd like to document this, but I'm not quite sure where.

[jonathanong]

wish we didn't have to support node < 4. then we could do for-of. sigh

[dead-horse]

for (var key in err.headers) is ok and have the similar performance with a small number of key.

  for in        x 2,743,592 ops/sec ±1.06% (90 runs sampled)
  Object.keys   x 3,294,202 ops/sec ±1.36% (91 runs sampled)

[thelinuxlich]

Besides, for-of is too slow at the moment

[jonathanong]

yeah, hmmm... we need a doc just on error handling

[jonathanong]

looks good to me.

people who would find interest in this, would you like to review? @niftylettuce @menems @dead-horse @tejasmanohar @cesarandreu @pensierinmusica @felixfbecker @ruimarinho @fixe

[dead-horse]

this.set(newHeaders);

[menems]

hi , I think the ability to add new header on error is a good point , but i am not totally agree about keeping already set header in that way. May be In addition we can have a global var like app.keepHeaders = true that keep all headers on error.

As I said, the problem with that approach is, the module responsible of keeping header have to know which headers are already set. (it's not always the case) and the goal is remove some boilerplate i think .

But I can't judge that because i haven't make a pull request for that issue (missing time sorry)
So maybe other have another opinion?

[PlasmaPower]

@menems Yeah this is a bit hacky. Maybe have err.headers and err.keepHeaders?

Edit: I understand your point now. Yeah I can see that being a problem. However, instead of a app.keepHeaders, I would like to see a ctx.keepHeadersOnError.

[dead-horse]

I'd rather remove keep header hack like err.headers.kept = true, people can just set err.headers.kept = this.response.get('kept').

Keep this feature simple and obvious.

[PlasmaPower]

@dead-horse Sounds good. I'll implement that in a bit. Would you like something to solve menems' problem, that the error thrower might not know which headers to keep? I think that a ctx.keepHeadersOnError object would be a good idea. Maybe a new name though, because that sounds like a boolean.

[PlasmaPower]

I also think that a ctx.keepHeadersOnError would be a good idea because it could help when errors are thrown in another library.

[dead-horse]

not really sure how much should we do in the default error handler, because lots of people already use third party error handlers that don't implement like this may cause some unexpected behavior. so we'd better keep the rules clearly enough and make third party error handlers follow these rules.

ctx.keepHeadersOnError maybe good, but I can't think any use case that we really need this for now.

[menems]

@dead-horse my use case, and i think it 's a standard one. I have a cors middleware and an authentication behavior, if i use ctx.throw(401). the headers of the response will be reseting and all cors headers too, so the client will scold me :) because the client want the cors headers even if you don't have permission to access a resource. that because on context error function headers is by default reset.

My only solution is to catch the error and set status to bypass the context.onerror.

app.use( (ctx, next) => next.catch(e => ctx.status = e.status))

It's a little annoying i think because status will be set by default ctx.onerror handler.

[PlasmaPower]

@menems the default error handler already sets the status: https://github.com/koajs/koa/blob/master/lib/context.js#L135

[menems]

@PlasmaPower Yes I know but it remove headers too https://github.com/koajs/koa/blob/master/lib/context.js#L121

[PlasmaPower]

@menems Oh, I thought that you were then throwing the error again after setting the status. Speaking of that, that might actually eliminate the use case for ctx.keepHeadersOnError:

app.use(function*(next){
  try {
    yield next;
  } catch (err) {
    err.headers = err.headers || {};
    err.headers['x-my-header'] = this.response.get('x-my-header');
    throw err;
  }
})

[PlasmaPower]

We seem to have a wiki page on error handling, should I make an edit there once this merges to document this?

I'm not quite sure on why we have koajs.com, the github wiki, and /docs/. Should we try to consolidate these?

[jonathanong]

koajs.com is built from the docs. we should put it in the docs!

[tj]

hahaha good call

[PlasmaPower]

@jonathanong Oh, I see. I thought they weren't the same since a recent change wasn't in there. I'll add a docs page for error handling then.

[PlasmaPower]

I've added documentation for this. Is there anything else that needs to be done?

[menems]

@PlasmaPower : ok so if I understand the goal of this pr is to pass header on error and throw it again?
ok no problem i'm going to continu to catch my error like this for keeping headers on response :D

app.use( (ctx, next) => next.catch(e => ctx.status = e.status))

ps: I'm still thinking this middleware should be unnecessary with a global var that keep headers for status != 5XX

[PlasmaPower]

@menems this is more aimed at koa libraries I think. They shouldn't implement their own error handlers. In your case, you should probably just make an app.on('error') error handler that does what you want. It will override ctx.onerror.

[menems]

@PlasmaPower : nop, i tried that before but you can override effectively app.onerror, but not ctx.onerrorbecause ctx.onerrorjust delegate the error to app.onerrorbut continue to remove headers and other stuff. other solutions are to extends Context object and override the ctx.onerror or, context can be like app and extends eventEmitter and have the same behaviour of application like here: https://github.com/koajs/koa/blob/v2.x/lib/application.js#L120

[PlasmaPower]

I see. Yeah since ctx.onerror is basically a try-catch at the start of the chain, then your middleware is practically an error handler. I guess that makes this the recommended way to make an error handler:

app.on(async (ctx, next) => {
  try {
    await next();
  } catch (err) {
    err.status = err.status || 500;
    ctx.body = 'An error occured: ' + err.status;
    ctx.status = err.status;
  }
})

[thelinuxlich]

If you do that, aren't you deoptimizing the code?

[PlasmaPower]

@thelinuxlich What do you mean?

[thelinuxlich]

try/catch deoptimizes V8 code

[PlasmaPower]

@thelinuxlich I wasn't aware of that. However, doing a bit of research, it would appear that it will not apply to the contents of the next call, so the impact is minimal. Is this the case? Also, if try-catch is to be avoided, you can always just do .catch on a promise:

app.on((ctx, next) => {
  return next().catch(function(err){
    err.status = err.status || 500;
    ctx.body = 'An error occured: ' + err.status;
    ctx.status = err.status;
  })
})

[PlasmaPower]

koa-compose@next try-catches middleware anyway, so I'm not sure any of that matters. https://github.com/koajs/compose/blob/next/index.js#L40

[thelinuxlich]

hmmm interesting

[jonathanong]

tried to see if removing the try/catch was faster... it was not. koajs/compose#49

[jonathanong]

i believe it only deoptimizes that function, anyway

[PlasmaPower]

it's actually faster w/ try/catch!

I propose adding 5 more try-catches for optimal performance /s

[felixfbecker]

+1 on more try/catches. Maybe add a finally block aswell?

[jonathanong]

anyways, this LGTM

[PlasmaPower]

I need to fix the docs before merging, it should be done soon. I was a bit confused on ctx.onerror and app.onerror.

[PlasmaPower]

Okay they should be good now.

[dead-horse]

LGTM

[PlasmaPower]

Small change to reflect current implementation: it('should keep headers specified in the error' -> it('should set headers specified in the error'.