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

fix(response-stream): fix response with no content doesn't correctly end the writable stream #206

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

stackia
Copy link
Contributor

@stackia stackia commented Feb 28, 2024

Description of change

Previously we are detecting HTTP status codes that indicate no body (status >= 100 && status < 200 || status === 304 || status === 204 || method === 'HEAD'). This is not great since every HTTP status code can have empty body. It's totally legit to have a response with 200 status code but empty body.

Actually, the root cause of stream not being correctly ended, is that we skipped the 0\r\n\r\n right after the HTTP header:

image

As you can see in the screenshot (captured from AWS Lambda logs), for responses with empty body, the first write contains both headers and the chunk terminator 0\r\n\r\n.

This PR tries to detect that chunk terminator immediately after the headers, so we can close the stream correctly.

Tested working on AWS lambda with examples from https://github.com/H4ad/serverless-adapter-examples (fastify-aws-stream and nestjs-aws-stream).

Here is a before & after comparison.

Before (and the request just hang until Lambda timeout):

image

After:

image

Pull-Request Checklist

  • Code is up-to-date with the main branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Added documentation inside www/docs/main folder.
  • Included new files inside index.doc.ts.
  • The new commits follow conventions outlined in the conventional commit spec

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (250221f) to head (bded8cf).

Additional details and impacted files
@@            Coverage Diff            @@
##           main     #206       +/-   ##
=========================================
+ Coverage      0   99.93%   +99.93%     
=========================================
  Files         0       65       +65     
  Lines         0     7792     +7792     
  Branches      0      658      +658     
=========================================
+ Hits          0     7787     +7787     
- Misses        0        5        +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@H4ad
Copy link
Owner

H4ad commented Feb 28, 2024

Hey, thanks for the detailed PR, looks very impressive!

I will take a deeper look at night, probably adding some tests just to ensure this is working fine but I will try to merge and release a new version as soon as possible.

Copy link
Owner

@H4ad H4ad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contribution!

@H4ad H4ad merged commit 115628a into H4ad:main Feb 29, 2024
7 checks passed
H4ad added a commit that referenced this pull request Feb 29, 2024
@H4ad H4ad mentioned this pull request Feb 29, 2024
@H4ad
Copy link
Owner

H4ad commented Feb 29, 2024

Released at v4.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants