Skip to content

Conversation

@punkeel
Copy link

@punkeel punkeel commented Jun 14, 2022

  • Make dispatch() an async function, and await every async function call.
    By removing Promise.xxx, we reduce the chance that an async function
    is called without (a)waiting for it to end, which in turn prevents
    uncaught exceptions.

  • Simplify code to let errors propagate upwards, without catching them.

  • Use 'dispatch.bind' in the recursion to remove a function from the
    stack trace. This binding trick was done in Koa:
    koajs/compose@13768ff

    To support this, we move the params-reset to the end of dispatch().

No tests were harmed in the making of this change.

I am opening this PR because I suspect some exceptions aren't caught by dispatch() and instead cause uncaught exceptions. I do not have a simple example of this happening, but I suspect it happens here:

route.handler(ctx, async () => {

route.handler is an async function, 8track does not await it, and it leaks. The try-catch doesn't see it either, as it's happening in the async world.... or maybe if (p) return p.then(() => ctx.response), we don't attach a .then(fn, fn) nor .catch(fn)...

- Make dispatch() an async function, and await every async function call.
  By removing Promise.xxx, we reduce the chance that an async function
  is called without (a)waiting for it to end, which in turn prevents
  uncaught exceptions.
- Simplify code to let errors propagate upwards, without catching them.
- Use 'dispatch.bind' in the recursion to remove a function from the
  stack trace. This binding trick was done in Koa:
  koajs/compose@13768ff

  To support this, we move the params-reset to the end of dispatch().

No tests were harmed in the making of this change.
@jrf0110
Copy link
Owner

jrf0110 commented Jun 28, 2022

Thanks for this! I've always disliked how inscrutable this function was. I'm going to play around with your change and will likely publish to a release candidate flag for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants