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 req end during response transmission #4264

Merged
merged 2 commits into from
Jun 28, 2021
Merged

Conversation

devinivy
Copy link
Member

Addresses #4262. On node v16 hapi would cut short any response that was transmitting while the request stream ended. This is not especially common, partly because the request stream is often used prior to transmitting the response, and partly because this case does not apply when the request stream is not read at all i.e. for GET requests.

I believe this change is sound and backwards compatible because on node versions prior to v16, both req and res 'close' events have identical meaning: the underlying connection shared by req and res has closed. For this reason, there is no need to add listeners to both req and res on node <v16.

Starting with node v16 the 'close' event for req no longer relates to the underlying connection, best documented at nodejs/node#38924 and nodejs/node#33035. This means that it is possible for req to 'close' prior to res, notably while transmitting res. The change in this PR is intended to take that into account. The test added here was failing on node v16 prior to this change.

See also #4263, which fixes a test that would have failed with the changes made in this PR.

@devinivy devinivy added the bug Bug or defect label Jun 23, 2021
Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

Great. I would probably have used server.inject() for the test but this works.

@devinivy
Copy link
Member Author

That's a good point. Originally it was not intentional to sidestep server.inject(), but rather result of working with the original repro provided in #4262. I did notice early on that the repro provided there using async iterators did not reproduce with server.inject(). I just looked into moving the new test to use it, and in fact shot isn't super useful to simulate this condition, as shot is more tied to the pre-v16 behavior (I am definitely looking at hapijs/shot#139 with new eyes). I could emit some events from req/res to simulate it manually, but in the end I am glad to have a real http test for this odd case. Thanks as always for the review! I hope/intend to land this soon.

Copy link
Member

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this seems to maintain the intent of the original handlers while coping with the subtle change in node 16. i think this is fine to land as-is, but we should really closely examine our event handlers at some point in the future

@devinivy devinivy merged commit 0d32840 into master Jun 28, 2021
@devinivy devinivy deleted the fix-req-end-during-res branch June 28, 2021 21:55
@devinivy devinivy self-assigned this Jun 28, 2021
@devinivy devinivy added this to the 20.1.5 milestone Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants