Error that gets handled by middleware is still passed on to next moddleware

[jacsamell]

When you have a chain of middlewares that implement onError, when the first middleware handles the error and calls next() without passing in the error as a parameter the next one should not be invoked according to the documentation.

However all onErrors in the chain are called regardless of whether the error has been passed into the next function.

[philprime]

I noticed the same issue when handling errors and setting them as a response:

export function RestErrorMapperMiddleware<T, R>(): middy.MiddlewareObject<T, R> {
  return ({
    onError: (handler: middy.HandlerLambda): Promise<void> => {
      const error = handler.error;
      let statusCode = INTERNAL_SERVER_ERROR;
      let errorMessage = error.message;

      if (error instanceof BadCredentialsException) {
        statusCode = UNAUTHORIZED;
        errorMessage = "Bad credentials."
      } 
      handler.response = new LambdaResult(statusCode, {
        statusCode: statusCode,
        body: JSON.stringify({
          message: errorMessage,
        })
      });
      return Promise.resolve();
    }
  });
}

All middlewares used afterwards are still called.

[lmammino]

Hello @jacsamell and @philprime, thanks a lot for reporting this.

This seems indeed a bug on how the error propagation is handled when using promises (or async-await).

Unfortunately, I currently have very little time to investigate this, but if you have some time to invest on this, this part of the code might be a good starting point.

Sorry if I can't be more helpful at this time.

[philprime]

Hi @lmammino, I just took a look at the code and if I change line 113 to the following, it does not execute the remaining error middlewares anymore:

middy/packages/core/index.js

Line 113 in a64218d

if (nextMiddleware) {

if (nextMiddleware && !instance.__handledError) {

Now I expected it to call the remaining onAfter middlewares outside of the one that handled the error, but I couldn't find anything to that topic.

If I extend the sample code of your documentation by another onAfter handler in middleware2, should that be called?

middleware1 = {
  onError: (handler) => {
    handler.response = { error: handler.error };
    return Promise.resolve();
    // Resolves to a falsy value
  }
}
middleware2 = {
  onError: (handler) => {
    return Promise.resolve(handler.error)
  },
  onAfter: (handler) => {
    // Should this be called?
  }
}
handler.use(middleware1).use(middleware2);

[lmammino]

Thanks a lot for spending some time on this issue @philprime! It might be worth to revisit the test suite and maybe submit a PR if you think that change is enough to stop the bug and does not have side effects on other behaviours.

Regarding your question, middleware2's onAfter should not be invoked by design. The idea is that once you are on the onError track you derailed outside the normal request/response flow and only onError handlers will be invoked (in order) giving them a chance to handle the error as needed.

If you want to have an additional step of error handling you should implement a onError also in middleware2 and keep propagating the error from middleware1 by doing return Promise.reject(handler.error).

[philprime]

I just created a pull request, only one test is failing:

If theres an error and one error middleware handles the error, the next error middlewares is executed

Is this test case actually doing what is intended?
Either I misunderstand its objective or it is contradicting to the design/documentation

[philprime]

I changed the test case as mentioned before and added correct handling for callbacks. See PR #497

[lmammino]

Thanks a lot @philprime and sorry again for the delay in reviewing this! I should be able to find some time during this Easter break.

[lmammino]

Sorry for all the confusion here. I haven't touched this in a while and I did not fully remember what was the intended flow and why.

Today I finally had some time to deep dive on this and get some more clarity (after messing up a release on master 😢 )

As mentioned in #497

As per the docs:

When a middleware handles the error and creates a response, the execution is still propagated to all the other error middlewares and they have a chance to update or replace the response as needed. At the end of the error middlewares sequence, the response is returned to the user.

This is needed for instance in the case of the HTTP error handler which will format the error in a correct response for the lambda HTTP integration. If this is combined with the response serializer it must be possible to convert this response to XML or other formats.

There are two ways in your custom middleware that you can decide not to perform any extra action in your error middleware if the error has been already handled:

1. Check the handler.__handledError property. If true another middleware has handled the error and produced a response. so you can wrap your middleware in an if statement and skip any additional action. Similarly, you can check if there's already a response in handler.response

Example:

middleware1 = {
  onError: (handler) => {
    handler.response = { error: handler.error };
    return Promise.resolve();
    // Resolves to a falsy value and will set instance.__handledError = true
  }
}

middleware2 = {
  onError: (handler) => {
    // will be called but you can check handler.response or handler.__handledError to 
    // decided whether to perform some additional action or not
  },
  onAfter: (handler) => {
    // Will NOT be called
  }
}

handler.use(middleware1).use(middleware2);

Now, it is true that the documentation section is not accurate (it is indeed wrong):

If onError promise resolves to a falsy value (null, undefined, false etc.), the error handling pipeline exits early and the response is returned without an error.

This needs to be fixed as for consistency with the callback version the code won't exit early.

I will make sure to update the documentation to:

If onError promise resolves to a falsy value (null, undefined, false etc.), the error handling pipeline continues and eventually the response is returned without an error.

This part is instead correct:

If onError promise rejects, the error handling pipeline exits early and the lambda execution fails.

So, this is a valid way of short-circuiting the error handling If needed when using promises or Async/Await.

Sorry again for all the mess and confusion... One of those days where I don't feel great about having enough time and focus to keep leading this project as good as needed 😞

[lmammino]

Closed by:

• 13d4085
• afe7446

[thejuan]

I think the docs are still incorrect on this?

Here, only middleware1.onError will be called. The rest of the error handlers will be skipped, and the lambda will finish normally and return the response. middleware2.onError will not be called.