-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ctx.throw() kills the server #882
Comments
// 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()) |
@EduardoRFS thank you, but I wrote it the description:
and questions are not about how to fix the code |
nop lack of |
Would it be sane to, in |
lack of an await/return is a bug. awaiting or returning it should fix it. otherwise, it's just a dangling promise |
@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 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 |
this is not a koa issue - you're going to have to bring this up with the people who make javascript |
@fl0w you mean parsing the function to see that next is awaited? |
@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 |
@jonathanong Scratch that. I actually ment counting the returns similarly to how |
Why lack of an await/return is a javascript bug? Is a functionality, you can run the promise and await after some instructions |
@Alexsey PR welcomed, but it shouldn't be enabled in production since it is a developer error (assuming it affects performance) |
@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? |
I always use // first middleware
app.use(async (ctx, next) => {
try {
await next()
} catch (e) {
handleErrorHere(e)
}
}) |
@dominhhai @Alexsey only do it if |
@jonathanong |
I'm moving the issue here, where it would be done. koajs/compose#74 |
This server
will blowup when request will get to the second middleware, and you will see it in the error
Error message blames request but the real problem is in the first middleware, where
next()
doesn't haveawait
before it. Lack ofawait
is probably a bug. The issue points are:ctx.body = 'ok'
and no error or warning would appears. Reaction on absents ofawait
should not depend on consequent code.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 andnext
is still pending with message says that this is the problemThe text was updated successfully, but these errors were encountered: