lukeautry / tsoa

Build OpenAPI-compliant REST APIs using TypeScript and Node
MIT License
3.54k stars 499 forks source link

Exceptions in async services called by the controller crashes Express (sometimes?) #1466

Closed mhvelplund closed 1 year ago

mhvelplund commented 1 year ago

Edit: found the reason; I had an exception originating in an unawaited promise 🤦‍♂️

I'm having some issues with the global error handler when using TSOA. I followed the guide here and created an error handler that looks like this:

app.use(function errorHandler( err: unknown, req: ExRequest, res: ExResponse, next: NextFunction): ExResponse | void {
  console.debug("Error handler:", err);

  if (err instanceof ValidateError) {
    return res.status(422).json({
      message: "Validation Failed",
      details: err?.fields,
    });
  }

  if (err instanceof AuthenticationError) {
    return res.status(401).json({
      message: "Unauthorized",
    });
  }

  if (err) {
    return res.status(500).json({
      message: "Internal Server Error",
    });
  }

  next();
});

Next, I created a controller that looks like this:

  @Security("api_key")
  @Get("{serviceName}")
  public async getLogs(
    @Request() request: any,
    @Path() serviceName: string,
    @Query() startTime?: number,
    @Query() endTime?: number
  ): Promise<LogRecord[]> {
    const logs = await this.logService.getServiceLogs(
      serviceName,
      request?.user,
      startTime ? startTime * 1000 : undefined,
      endTime ? endTime * 1000 : undefined
    );

    const records = logs.map((log) => {
      return { timestamp: log.timestamp!, message: log.message!};
    });

    return records;
  }

Everything worked fine for a while, but hen I notice that in some cases the error handler is not called. I can be more specific if needed, but essentially if this.logService.getServiceLogs() throws an exception it crashes express.

Express logs:

[0] Example app listening at http://localhost:3000
[0] /home/qmxh/projects/ssad/management-api/server/src/registry/registryService.ts:104
[0]         throw new AuthenticationError(`No ApiKeyArn for ${serviceName}`);
[0]               ^
[0] AuthenticationError: No ApiKeyArn for aaa
[0]     at RegistryService.<anonymous> (/home/qmxh/projects/ssad/management-api/server/src/registry/registryService.ts:104:15)
[0]     at Generator.throw (<anonymous>)
[0]     at rejected (/home/qmxh/projects/ssad/management-api/server/src/registry/registryService.ts:18:65)
[0]     at processTicksAndRejections (node:internal/process/task_queues:95:5)
[0] [nodemon] app crashed - waiting for file changes before starting...

But if I change the controller to just throw everytime, the error handler works, and a 401 HTTP response.:

Simplified controller:

  @Security("api_key")
  @Get("{serviceName}")
  public async getLogs(/*...*/): Promise<LogRecord[]> {
    throw new AuthenticationError(`No ApiKeyArn for ${serviceName}`);
  }

Express logs:

[0] Example app listening at http://localhost:3000
[0] Error handler: AuthenticationError: No ApiKeyArn for aaa
[0]     at LogController.<anonymous> (/home/qmxh/projects/ssad/management-api/server/src/logs/logController.ts:74:11)
[0]     at Generator.next (<anonymous>)
[0]     at /home/qmxh/projects/ssad/management-api/server/src/logs/logController.ts:20:71
[0]     at new Promise (<anonymous>)
[0]     at __awaiter (/home/qmxh/projects/ssad/management-api/server/src/logs/logController.ts:16:12)
[0]     at LogController.getLogs (/home/qmxh/projects/ssad/management-api/server/src/logs/logController.ts:49:16)
[0]     at /home/qmxh/projects/ssad/management-api/server/build/routes.ts:63:50
[0]     at Generator.next (<anonymous>)
[0]     at fulfilled (/home/qmxh/projects/ssad/management-api/server/build/routes.ts:5:58)
[0]     at processTicksAndRejections (node:internal/process/task_queues:95:5)

I read through the Express documentation and it seems you are not allowed to throw exceptions in async functions. Instead you are supposed to call the next() function with them. The TSOA controller doesn't give me that option, and the odd thing is that I'm 90% sure that it actually caught some of the other exceptions before, inside the async code.

So my question is, are there any limitations on the global error handling that I'm unaware of? Is the handler supposed to be able to trap exceptions thrown inside async functions, and if so, why might it be failing here?

Steps to Reproduce

See above.

Context (Environment)

Version of the library: 5.1.1 Version of NodeJS: 18.16.0

Detailed Description

See above.

github-actions[bot] commented 1 year ago

Hello there mhvelplund 👋

Thank you for opening your very first issue in this project.

We will try to get back to you as soon as we can.👀

mhvelplund commented 1 year ago

I feel like an idiot. After wasting 2+ hours of my weekend on this and creating this issue, I found a ugly workaround and moved on. A little later I ran the linter and found the reason for the previous problem. The root exception originated in an unawaited async method.

Would it be possible to add a note to the starter guide saying that exceptions in unhandled promises escape the error handler and crash Express?

(obviously this issue can be closed)

WoH commented 1 year ago

This is one of the major caveats of express (unability to properly deal with async/await).

The docs repo is here if you want to add a small note to the guide: https://github.com/tsoa-community/docs