serverless-adapter icon indicating copy to clipboard operation
serverless-adapter copied to clipboard

Responses will never be send when trailing headers are set

Open florian-g2 opened this issue 1 year ago • 1 comments

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

  • All

florian-g2 avatar Sep 09 '24 20:09 florian-g2

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!

H4ad avatar Sep 09 '24 22:09 H4ad