H4ad / serverless-adapter

Run REST APIs and other web applications using your existing Node.js application framework (NestJS, Express, Koa, tRPC, Fastify and many others), on top of AWS, Azure, Huawei and many other clouds.
https://serverless-adapter.viniciusl.com.br/
MIT License
132 stars 8 forks source link

Responses will never be send when trailing headers are set #262

Open florian-g2 opened 1 week ago

florian-g2 commented 1 week ago

While implementing my other PR, I noticed that trailing headers are not supported and using them blocks the response until the timeout of the serverless runtime. The used constant endChunked = '0\r\n\r\n'; does not respect trailing headers. The trailing headers may be added in between the two CRLFs. I do not suggest to fix it right away, it should just be kept on radar and be documented. As far as I can tell, trailing headers are aside from checksums not very broadly used.

Current Behavior

A response will never finish when trailing headers are set.

Expected Behavior

Response with trailing headers are sent normally.

Steps to Reproduce the Problem

it('should return the correct bytes of chunked stream with trailing headers', async () => {
    const app = express();
    const file = readFileSync(join(__dirname, 'bitcoin.pdf'));

    app.get('/', (_, res) => {
      const readable = createReadStream(join(__dirname, 'bitcoin.pdf'));

      res.statusCode = 200;
      res.setHeader('content-type', 'application/pdf');
      res.setHeader('Trailer', 'Content-MD5');
      res.flushHeaders();

      readable.pipe(res);

      // add trailing headers
      res.addTrailers({ 'Content-MD5': '7895bf4b8828b55ceaf47747b4bca667' });
    });

    const expressFramework = new ExpressFramework();

    const handler = awsStreamHandler.getHandler(
      app,
      expressFramework,
      adapters,
      resolver,
      binarySettings,
      respondWithErrors,
      logger,
    );

    const event = createApiGatewayV2('GET', '/', {}, { test: 'true' });
    const context = { test: Symbol('unique') };

    const writable = new WritableMock();

    // check if handled in time
    let handledInTime = true;
    await Promise.race([
      handler(event, writable, context),
      new Promise(resolve => {
        setTimeout(() => {
          handledInTime = false;
          resolve(undefined);
        }, 1000);
      }),
    ]);

    expect(handledInTime).toBeTruthy();

    expect(getCurrentInvoke()).toHaveProperty('event', event);
    expect(getCurrentInvoke()).toHaveProperty('context', context);

    const finalBuffer = Buffer.concat(writable.data);

    expect(Buffer.byteLength(finalBuffer)).toBe(Buffer.byteLength(file));
});

Environment

H4ad commented 1 week ago

Interesting, I will try take a look when I had more time.

In the past, I almost rewrite the entire implementation to send data directly to AWS instead of relying on the AWS abstraction that is broken/bad.

But thanks for reporting!