Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
florian-g2 opened this issue Sep 9, 2024 · 1 comment
Open

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

florian-g2 opened this issue Sep 9, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@florian-g2
Copy link
Contributor

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 florian-g2 added the bug Something isn't working label Sep 9, 2024
@H4ad
Copy link
Owner

H4ad commented Sep 9, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants