Repository navigation
Backport fix from v2: Do not swallow calls to res.end() - #298
Open
papandreou wants to merge 2 commits into
Open
papandreou wants to merge 2 commits into
papandreou wants to merge 2 commits into
Conversation
* Add tests demonstrating scenarios under which the original res.end does not get called * Ensure stream gets closed and response gets ended when it is finished * Only call the original res.end() a single time * Avoid using the on-finished package * Use finished() helper from node:stream * Import finished() helper directly from node:stream Co-authored-by: Sebastian Beltran <bjohansebas@gmail.com> * Use finished() helper directly instead of nodeStream.finished() * Adjust stream pause / resume logic instead of messing with res.end() calls directly * Revert addition of endOnce() wrapper * Add unit test around one more alternate sequence of events that seems reasonable to test in addition to the other ones * Add test around avoiding 'write after end' errors * Avoid writing into the response after it is finished * Add a comment explaining why the compression stream gets resumed when the response finishes * chore: use isFinished of on-finished --------- Co-authored-by: Sebastian Beltran <bjohansebas@gmail.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
krzysdz
suggested changes
Oct 27, 2026
krzysdz
left a comment
krzysdz
left a comment
Contributor
There was a problem hiding this comment.
I can't run/unblock the CI for you, but Node.js 0.8 does not support const or destructuring assignments, so these have to be changed for 1.x.
Use on-finished instead of stream.finished, avoid const/destructuring and template literals, and call the original res.end() when the response closes, since the destroyed stream never emits 'end'. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Author
Ah, good shout. Tried to fix that now 🙏 |
This file contains hidden or 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
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.
#188 was merged to
v2on Apr 16, 2025, but it seems like it might be a while until we see an actual 2.0.0 release from this repo. In the mean time, I suggest that we backport this fix so that it can be included in a 1.x release. Pretty please! 🤗