http: http client response should always emit close when finished #17397
Conversation
|
@ronag Would you be able to look at the test coverage for this code and make sure it properly covers this behavior, or else would you be able to add new tests? |
|
@nodejs/http |
|
@maclover7: Sorry, not sure how to build a test for this case. The tests are still a bit over my head. |
|
@nodejs/http @nodejs/testing anyone to provide some quick help? |
lib/_http_client.js
Outdated
| // Socket closed before we emitted 'end' below. | ||
| res.emit('aborted'); | ||
| res.on('end', function() { | ||
| res.emit('close'); |
There was a problem hiding this comment.
Since you're here: this.emit('close') - saves a closure over res.
lib/_http_client.js
Outdated
| res.emit('close'); | ||
| }); | ||
| res.push(null); | ||
| } else { | ||
| res.emit('close'); |
There was a problem hiding this comment.
Since res.readable == false, does that mean an 'end' event has already been emitted?
There was a problem hiding this comment.
end will never be called if not readable at this point.
|
@ronag about writing a test: The tests here work the same as if you would write code for your own project. So what you want to do is to write some code that would actually fail without this change. I guess you should know how to get there as you found out that it does not always emit a close when finishing. The only difference here is that you have to include the "common" part as first require statement as done in every test file in the test folder. You probably want to add a new file in the "parallel" part. In addition to that you have some extra helper functions that you find in the common part and that also has a individual documentation. But you should probably not need that at all. To run the tests you first have to build the node executable (I guess you use a unix system, so run I hope that helps? :-) |
|
In addition to @BridgeAR's excellent guide, you could also take inspiration from the test here https://github.com/nodejs/node/blob/master/test/parallel/test-http-client-close-event.js and create a new one that hits the code path that you've modified. |
|
@ronag would you mind taking another look? :-) |
|
Sorry guys... I’m swamped... feel free to take over |
|
@ronag too bad. And as a note: we are not all guys. @apapirovski @jasnell would either of you be willing to write a test for this? |
|
@ronag ... This isn't the place for that debate. |
|
@nodejs/http @nodejs/http2 would someone be so kind and write a small test case for this? |
|
@trivikr thanks a lot! :-) And yes, in this case I recommend to cherry-pick this commit and then add your changes and open a new PR for it. |
|
@trivikr do you still want to follow up on this? :-) |
|
@BridgeAR The code is lying in my private branch since last month, let me add the tests and post a PR :-) |
|
Closing this in favor of new PR #20043 |
fixes #17352
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http