Allow for res to have already closed during transmission#4234
Merged
Allow for res to have already closed during transmission#4234
Conversation
nlf
approved these changes
Mar 12, 2021
Member
nlf
left a comment
nlf
left a comment
There was a problem hiding this comment.
i wasn't able to identify a better way to track this either. this is isolated enough and clear enough that i think it's reasonable 👍
test/transmit.js
Outdated
| request.raw.res.once('close', () => close.attend()); | ||
|
|
||
| client.destroy(); // Will trigger abort then close. Prior to node v15.7.0 the res close came | ||
| await close.work; // asynchronously after req abort, but since then it comes in the same tick. |
Member
There was a problem hiding this comment.
total nit, but the multiline comment to the right here should be above these two lines. as it is, it makes it look like the second line of the comment is regarding the second line of the code which is not the case
devinivy
commented
Mar 12, 2021
This was referenced Mar 16, 2021
Merged
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
2 participants
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.
There's a fun story here!
In nodejs/citgm#436 it was brought to our attention that node v15.7.0 caused a test to reliably fail in hapi's test suite. I found that the test fails due to the relative timing of request
abortversus responseclosetiming: prior to node v15.7.0 the response would close in a later tick than the request would abort, and now these occur in the same tick. This means that during hapi's transmission step, where it would typically still assume that the request or response would still have some events to emit (namely, a responseclose), that there would be no more event fromreqandres. The first thing I did was adjust this test to remove the timing discrepancy across node versions, so that it always takes the most aggressive approach that we see occurring in node v15.7.0 (always aborts and closes during marshaling).I searched for a nice node-builtin way to detect this case, but I didn't find anything that stuck. The most promising one was to check for
res.destroyed, but it turns out that this was set in node v15, but not in node v14. So I decided to add some state to hapi's request that tracks whether the response is closed, which to the best of my knowledge indicates that we can't expect to see any more events fromreqandres.I ensured this code path continued to go through hapi's ending routine as though
closewere emitted, even though it ends-up being a no-op. Honestly, adding some more state torequestto track this and triggering afinishonreswas not my favorite approach, but it was hard for me to find something more appropriate. Certainly open to suggestions. One aspect that I wouldn't mind some review on is whether the new event listener could introduce some kind of leak since it doesn't stop referencingrequestas the other listeners do through_eventContext— I think we're all good there, but it was on my mind. @kanongil of course no pressure, but if you do have time to take a look, I think you are one of the people best-suited to review this area of hapi and any input you have would be appreciated.