guard against unhandled next()s in development

[jonathanong]

koajs/koa#882

[PlasmaPower]

I'm working on this now.

[PlasmaPower]

There are two ways to approach this: should I try to resolve the problem while printing out an error message, or should I just throw an error? Right now I'm leaning towards the latter, the former isn't always possible anyways.

[PlasmaPower]

This is proving to be more complicated then expected. Consider the following examples (where longAction returns a promise that takes a while to resolve):

compose([(ctx, next) => next(), (ctx, next) => { ctx.body = 'hello'; return longAction(); }])

and

compose([(ctx, next) => longAction(), (ctx, next) => { ctx.body = 'hello'; }])

From compose's point of view, we can't tell those apart. We could try to "tag" the promise (and modify .then and .catch to keep the tag), but then somebody calls Promise.all and we loose the tag. Loosing the tag means that we think the middleware failed to return next, which errors. That's a lot worse than thinking it succeeded.

I think the best we can do is simply check if the middleware resolves before next. However, this can create a race condition, which could be very dangerous if e.g. a user doesn't set their node environment in prod.

[JeffRMoore]

I have a working test for this, I haven't gotten around to updating the PR yet. You can see here

This jest test passes:

  it('should throw if next() or skipNext() not called', async () => {
    const middleware = [
      async (ctx, next, skipNext) => {
      }
    ];

    expect.assertions(1);
    try {
      await compose(middleware)({}, terminate, terminate);
    } catch (e) {
      expect(e).toBeInstanceOf(Error);
    }
  });

If you don't have skipNext functionality, you can NEVER tell the difference between intentionally not calling next and forgetting to call next. Without that, there is not much point in any kind of check. Without the signature change, there is simply no way to detect unhandled.

Adding the promise chain check cuts performance significantly. (125k req/s vs 330 req/s)

I've also experimented with using the promise chain check to find out of order promise resolution. It really doesn't buy you much, though. I think the technique of injecting a known value to the bottom of the chain and then testing for it at the top (enabled by skipNext) is fast and verifies the entire chain is intact.

The benchmark shows why tagging and anything that doesn't instrument the promise chain will fail:

  const fn = (ctx, next) => {
    return logic().then(next).then(logic)
  }

[PlasmaPower]

If you don't have skipNext functionality, you can NEVER tell the difference between intentionally not calling next and forgetting to call next

This issue is about forgetting to return next, not forgetting to call next.

[PlasmaPower]

The benchmark shows why tagging and anything that doesn't instrument the promise chain will fail:

That's why I say:

and modify .then and .catch to keep the tag

However, I agree that it is still really unreliable.

[JeffRMoore]

Sorry for missing that point. I'm a little rushed today. I spent a lot of time on this previously and mostly just wanted to quickly dump what I had so you didn't duplicate effort or might find something in what I did.

[JeffRMoore]

This is where I had gotten to in detecting out of order resolves, against this branch

Initialize a bottom up counter by lastCalled

    let minResolved = middleware.length + 1;

After we know we have a promise:

          return result.then ((value) => {
            if (i > minResolved) {
              throw new Error('Middleware Promise chain resolved out of order');
            }
            minResolved = i;
            return value;
          })

No tests to share yet, sorry.

[PlasmaPower]

I already have code to detect out of order resolves, but that can easily create a race condition. That's very dangerous if a user e.g. doesn't set their node env in production.

[JeffRMoore]

@PlasmaPower I'm not understanding the race condition possibility. Can you share an example?

[PlasmaPower]

@JeffRMoore

compose([(ctx, next) => doStuff().then(doStuff), (ctx, next) => doStuff()]);

Where the promise returned by doStuff takes a somewhat variable amount of time to resolve (for instance updating a row in a database).

Depending on the timing, it may occur e.g. 1% of the time, noticeable in production but not dev.

[JeffRMoore]

This issue is about forgetting to return next, not forgetting to call next.

compose([(ctx, next) => doStuff().then(doStuff), (ctx, next) => doStuff()]);

I'm confused. The first middleware doesn't call next.

[PlasmaPower]

Oops. Sorry about that. I meant:

compose([(ctx, next) => { next(); return doStuff().then(doStuff) }, (ctx, next) => doStuff()]);

[JeffRMoore]

@PlasmaPower Thanks for the update.

There's a clear break in the promise chain. I believe the minResolved code I posted will detect if these resolve out of order. Two promise chains will be created. If the detached lower one resolves first, it will be turned into a rejection, which clearly will be "unhandled." I think the minResolved code is safe for production. It doesn't change that there is a break in the chain, it just invalidates the broken chain early. If some other error occurred, the detached chain would result in an unhandled rejection anyway.

If the promises all resolve in the right order and no error occurs to trigger an unhandled rejection, the code is still wrong because if an error did occur in the detached promise chain it would result in an unhandled rejection. Detecting out of order promises does not necessarily give early warning of the error, unless the promises consistently resolve out of order (or often enough to trigger errors).

I've tried constructing some test cases around these scenarios. A lot of times the promises just happen resolve in order even when the chain is broken. It would be nice to have confidence in tests without having to noodle over exotic async scenarios.

Injecting a known value at the end of the promise chain and testing for it at the top can tell you instantly that the chain is broken. Its fast and easy and you don't have to think much about async, just get good conditional test coverage and use the framing code in your tests (inject sentinel, test return result at the end).

I like response as a sentinel, because its something you can actually do something useful with it up and down the chain, but a Symbol or anything unique would work.

That's why I gave up on out of order detection. It's much slower in the benchmark, creates extra promises and still doesn't necessarily catch errors in test cases.

The catch being that the case of intentionally breaking the chain requires some additional support and a perfect elegant solution to that case doesn't seem to have emerged.

I lean toward supporting catching errors. Even the best programmers have bad days.

[JeffRMoore]

Speaking of bad days, I just realized the out of order resolve detection code above doesn't cover cases where errors occur.

          return result.then ((value) => {
            if (i > minResolved) {
              throw new Error('Middleware Promise chain resolved out of order');
            }
            minResolved = i;
            return value;
          }, (error) => {
            if (i > minResolved) {
              throw new Error('Middleware Promise chain resolved out of order');
            }
            minResolved = i;
            throw error;
          })
        }