fix: add a check if the res is destroyed before sending response #10106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the case of using `curl` or a similar tool on the command line, or if the client decides to end the request for a streamed file early without the check we would end up causing a server crash. Now, we will just not send the response as the client has already decided how to move on. fix: nestjs#10105
I had to use node's `http` module for this because I could not get `supertest`s `abort` method to work properly. With the raw `http` module I was able to use `req.destroy()` to cancel the request early. We now see the error from a premature closure, but are still able to make extra requests afterwards
Pull Request Test Coverage Report for Build 6bd812e5-d78a-48a1-b3ea-7496b7f1d761
💛 - Coveralls |
thiagomini
reviewed
Aug 12, 2022
As we make use of the `http` module instead of axios or supertest we have tomake use of some rather low level code. Using this utils file we can have clearer names of what is happening in each request to make it easier to follow what's happening. I've also added in some comments about why each part is the way it is for clarity.
thiagomini
approved these changes
Aug 13, 2022
micalevisk
reviewed
Aug 13, 2022
micalevisk
approved these changes
Aug 14, 2022
Tony133
reviewed
Aug 14, 2022
Tony133
approved these changes
Aug 14, 2022
15 tasks
|
LGTM |
|
Hi, I am on NestJs cli v9.0.0.. I am getting this issue while streaming video files. Any solutions to fix this issue.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
If a client currently closes the request before the stream finishes we get an error about not being able to send headers to a response that is already sent. It should be noted that this is express only, fastify does not have this issue
Issue Number: #10105
What is the new behavior?
In the case of using
curlor a similar tool on the command line,or if the client decides to end the request for a streamed file early
without the check we would end up causing a server crash. Now, we will
just not send the response as the client has already decided how to
move on.
Does this PR introduce a breaking change?
Technically yes due to the fact that the server now no longer dies, but that is the expected case, so I believe this should be fine as a
patchrather than amajorbump. The fact that we actually now prevent a crash is the good thing 🐱Other information