aws / aws-sdk-js

AWS SDK for JavaScript in the browser and Node.js
https://aws.amazon.com/developer/language/javascript/
Apache License 2.0
7.59k stars 1.55k forks source link

NestJS crashes when AWS-SDK throws any type of Error #4123

Closed LuisOfCourse closed 4 weeks ago

LuisOfCourse commented 2 years ago

Describe the bug I have got the same problem but with every call that I purposely try to fail:

I was trying to catch the error when a wrong key was inserted (getObject) but I noticed that everything that throws this repo is crashing the entire server.

Expected Behavior The error should be catched by nestjs automatically like other errors in other packages (typeorm ex.). No custom code is needed to catch the errors in nestjs.

Current Behavior All errors thrown by the package crashes the instance of nestjs.

Reproductio Steps: Below the minimal code to reproduce the issue.

controller @Get('test/:test') async getFiles(@Param() { test }, @Res() res: Response) { const file = await this.filesService.getFiles(test); file.createReadStream().pipe(res) }

service async getFiles(test) { return await this.fileAwsS3Service.downloadS3( 'test/' + test + 'file.txt', ); }

aws s3 service

downloadS3(filePath: string) {
  const s3 = this.getS3();
  const params = {
    Bucket: this.configService.get('aws.s3.bucket'),
    Key: filePath,
  };
  return s3.getObject(params);
}

getS3() {
  return new S3({
    region: this.configService.get('aws.s3.region'),
    accessKeyId: this.configService.get('aws.s3.accessKeyID'),
    secretAccessKey: this.configService.get('aws.s3.secret'),
  });
}

If I try to purposely mess up the url/key I get 'key doesn't exists' but the application crashes. This example is the clean code with no try catch nor handlers.

Ill share more examples on test that I made to catch the errors. Like try/catch and eventHandlers.

SDK version used: 2.1151.0 Environment detail: Windows OS, node.js v16.13.2, nestjs 8.2.4

REPOST: Originally posted by @LuisOfCourse in https://github.com/aws/aws-sdk-js/issues/3846#issuecomment-1152930866

ajredniwja commented 2 years ago

Hi @LuisOfCourse, thanks for opening this issue would you be able to give me some more information mentioned in the issue template

md-shah commented 2 years ago

@LuisOfCourse It seems like you don't have a Common Exception Handler in NestJS. If yes, Can you please post it here?

LuisOfCourse commented 2 years ago

@ajredniwja Updated with the additional data you required.

@md-shah Exception Handler in this case does exists and it is the default one of nestjs.

... Out of the box, this action is performed by a built-in global exception filter, which handles exceptions of type HttpException (and subclasses of it). When an exception is unrecognized (is neither HttpException nor a class that inherits from HttpException), the built-in exception filter generates the following default JSON response ...

If I throw an error like this: @Get('test/:test') async getFiles(@Param() { test }) { throw new Error() } The application does not crash, instead nestjs automatically catches it!

md-shah commented 2 years ago

Hi @LuisOfCourse,

I tried to reproduce the same on a NestJS project using Fastify under the hood and without a custom exception handler.

One thing is that. the project didn't crash (At least I couldn't reproduce the same scenario where the app crashes), But the error is not getting thrown properly. But I was able to handle it by converting the function to promise.

Code without callback/promise

downloadS3(filePath: string) {
    const s3 = this.getS3();
    const params = {
      Bucket: this.config.S3.imagesBucket,
      Key: filePath,
    };
    return s3.getObject(params);
  }

Response for the same

  [Nest] 38979  - 14/06/2022, 13:59:37   ERROR [ExceptionsHandler] Converting circular structure to JSON
    --> starting at object with constructor 'Request'
    |     property 'response' -> object with constructor 'Response'
    --- property 'request' closes the circle
TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Request'
    |     property 'response' -> object with constructor 'Response'
    --- property 'request' closes the circle
    at JSON.stringify (<anonymous>)
    at serialize (/home/USER/node_modules/fastify/lib/reply.js:707:15)
    at preserializeHookEnd (/home/USER/node_modules/fastify/lib/reply.js:371:15)
    at preserializeHook (/home/USER/node_modules/fastify/lib/reply.js:356:5)
    at _Reply.Reply.send (/home/USER/node_modules/fastify/lib/reply.js:188:7)
    at FastifyAdapter.reply (/home/USER/node_modules/@nestjs/platform-fastify/adapters/fastify-adapter.js:49:29)
    at RouterResponseController.apply (/home/USER/node_modules/@nestjs/core/router/router-response-controller.js:14:36)
    at /home/USER/node_modules/@nestjs/core/router/router-execution-context.js:175:48
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at /home/USER/node_modules/@nestjs/core/router/router-execution-context.js:47:13

{
    "statusCode": 500,
    "message": "Internal server error"
}

The app didn't crash in the above scenario. But when I converted to promise, it worked and got an expected response.

Code with promise

  downloadS3(filePath: string) {
    const s3 = this.getS3();
    const params = {
      Bucket: this.config.S3.imagesBucket,
      Key: filePath,
    };
    return s3.getObject(params).promise();
  }

Response for above code

{
    "statusCode": 404,
    "message": "The specified key does not exist."
}

Tl;dr - Add .promise() and was working as expected.

LuisOfCourse commented 2 years ago

@md-shah thank you for your quick response. Yes, I can confirm. By using the promise you wont face this problem anymore. But this force me to use the promise for each transaction I want to do (with this package). With the method I used above I'm "streaming the file stream" of aws. So I send packeges as they arrives and this means less memory usage. In your case I need to have the complete file before streaming it back to the client. Anyway nestjs won't crash, but won't accept any other call either.

So can you confirm that the this package blocks every other api transaction when error is thrown (and promise is not used)?

md-shah commented 2 years ago

@LuisOfCourse This looks interesting. Yes, the issue exists with the stream and it kill the process as well.

LuisOfCourse commented 2 years ago

The full error is reported here:

C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\services\s3.js:711
      resp.error = AWS.util.error(new Error(), {
                                  ^
NoSuchKey: The specified key does not exist.
    at Request.extractError (C:\Users\User/Documents\Projects\test\test-api\node_modules\aws-sdk\lib\services\s3.js:711:35)
    at Request.callListeners (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\sequential_executor.js:106:20)
    at Request.emit (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\sequential_executor.js:78:10)
    at Request.emit (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\request.js:686:14)
    at Request.transition (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\request.js:22:10)
    at AcceptorStateMachine.runTo (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\state_machine.js:14:12)
    at C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\state_machine.js:26:10
    at Request.<anonymous> (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\request.js:38:9)
    at Request.<anonymous> (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\request.js:688:12)
    at Request.callListeners (C:\Users\USER\Documents\Projects\test\test-api\node_modules\aws-sdk\lib\sequential_executor.js:116:18)`
gisioraelvis commented 2 years ago

@ajredniwja or @md-shah Any updates on work progress towards resolving this issue?

Burgov commented 1 year ago

I have the same issue. Had to convert

    asset
      .createReadStream()
      .pipe(res);

into

    asset
      .createReadStream()
      .on('error', function () {
        res.sendStatus(404);
      })
      .pipe(res);

to prevent my app from crashing and restarting, but it's a bit annoying that it is necessary.

nikolaevn commented 1 year ago

@Burgov it looks like a bug, see #4333

toume commented 1 year ago
aBurmeseDev commented 4 weeks ago

Hey everyone - sorry for the delay response.

The issue you're facing is not specific to the SDK. You need to implement explicit error handling if you want to use createReadStream. To address this, add an error event handler for the stream, like this:

var fs = require("fs");
const s = fs.default.createReadStream('/this-file-not-exits');
s.on('error', (err) => {
  // Handle the error appropriately
  console.error('Error reading file:', err);
});

Related post in Node repo: https://github.com/nodejs/node-v0.x-archive/issues/8558.

Closing since this isn't SDK related issue.