Koa: ctx.throw() kills the server

[Alexsey]

This server

const app = new (require('koa'))()

// first middleware
app.use(async (ctx, next) => {
  next()
})

// second middleware
app.use(async ctx => {
  ctx.throw(400)
})

app.listen(3000)

process.on('unhandledRejection', reason => {
  throw reason
})

will blowup when request will get to the second middleware, and you will see it in the error

BadRequestError: Bad Request
    at Object.throw (...\node_modules\koa\lib\context.js:91:23)
    at .../server.js:10:3
    at Generator.next (<anonymous>)
    at step (...\server.js:3:191)
    at ...\server.js:3:437
    at ...\server.js:3:99
    at app.use (.../server.js:9:1)
    at dispatch (...\node_modules\koa-compose\index.js:44:32)
    at next (...\node_modules\koa-compose\index.js:45:18)
    at .../server.js:6:3

Error message blames request but the real problem is in the first middleware, where next() doesn't have await before it. Lack of await is probably a bug. The issue points are:

1. Should it blowup the server?
2. Every thing would be ok if second middleware would just ctx.body = 'ok' and no error or warning would appears. Reaction on absents of await should not depend on consequent code.
3. Reaction should be as soon as possible. Seen the crash on ctx.throw() in the other part of a project is very confusing.

Edit: Possible solution could be to throw if next has been called, middleware is resolved and next is still pending with message says that this is the problem

[EduardoRFS]

// first middleware
app.use(async (ctx, next) => {
  next()
})

here is the problem, you need to await or return next(), when you dont do that, the promise chain is ended, just change to

app.use(async (ctx, next) => {
  await next()
})
// or a simple arrow function
app.use((ctx, next) => next())

[Alexsey]

@EduardoRFS thank you, but I wrote it the description:

Lack of await is probably a bug

and questions are not about how to fix the code

[EduardoRFS]

nop lack of await is not a bug, is a part of promise chain, you can use return, is how koa work

[fl0w]

Would it be sane to, in koa-compose, make sure that the return is present (AFAIK async in ES@next always returns a Promise that resolves anyway)? Mini safeguard against newbie buggs that doesn't affect run-time performance?

[jonathanong]

lack of an await/return is a bug. awaiting or returning it should fix it. otherwise, it's just a dangling promise

[Alexsey]

@jonathanong Of course lack of await/return is a bug. The problem is that this bug in user code causes unexpected behavior of koa itself - call to ctx.throw produce unhandled promise rejection. And the biggest problem that this "breakage" is deferred and error message become unclear.

On the other hand there may be a way to detect that the system obtain invalid state and do not wait until it breaks

Generally there should be no way for users to silently set the system to invalid state

[jonathanong]

this is not a koa issue - you're going to have to bring this up with the people who make javascript

[jonathanong]

@fl0w you mean parsing the function to see that next is awaited?

[Alexsey]

@jonathanong I propose solution that may be easily implemented with current functionality of javascript in my initial post from the very begging. Please, read the last part of it

[fl0w]

@jonathanong Scratch that. I actually ment counting the returns similarly to how koa-compose makes sure next isn't called multiple times - but that doesn't necessarily mean it's forgotten. My bad.

[EduardoRFS]

Why lack of an await/return is a javascript bug? Is a functionality, you can run the promise and await after some instructions

[jonathanong]

@Alexsey PR welcomed, but it shouldn't be enabled in production since it is a developer error (assuming it affects performance)

[Alexsey]

@jonathanong ok, I'll look what I can do. May be there would even be a way to do this without performance hit. But if it would - what do you mean under "enabled in production"? Are there some other options in koa that may be shown with some flag or arg?

[dominhhai]

I always use try catch for the first middleware as:

// first middleware
app.use(async (ctx, next) => {
  try {
     await next()
  } catch (e) {
     handleErrorHere(e)
  }
})

[jonathanong]

@dominhhai that's not going to do much if next() returns a promise you changed your code

@Alexsey only do it if process.env.NODE_ENV !== 'production'

[dominhhai]

@jonathanong await a returned-promise from next() is OK, thought.
Except for the unhandled exception inside next().
It's bad if not try catch a await promise, IMO.

[jonathanong]

I'm moving the issue here, where it would be done. https://github.com/koajs/compose/issues/74