http: don't double-fire the req error event#14333
http: don't double-fire the req error event#14333fengmk2 wants to merge 1 commit intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
These asserts aren't necessary since common.mustCall() already checks that the functions are called exactly the specified number of times (with 1 being the default).
There was a problem hiding this comment.
Instead of using a fixed port as-is like this, consider using a custom http.Agent that returns an error in createConnection(). For an example of this, see this commit. This way we don't end up having a potentially flaky test in the future.
03aa455 to
5e9c272
Compare
5e9c272 to
934cb88
Compare
There was a problem hiding this comment.
These count variables and console.log()s are no longer needed either IMHO.
934cb88 to
ba8e1b7
Compare
|
@mscdex @dead-horse Thanks for the quickly review, all changed. |
cjihrig
left a comment
There was a problem hiding this comment.
LGTM with a few suggestions.
There was a problem hiding this comment.
I don't think you have to include these first 21 lines in new files.
There was a problem hiding this comment.
const req = http.get({ host }); should fit comfortably on one line.
There was a problem hiding this comment.
You should probably verify that the error received here is the same one that is thrown. If you declare the error outside of the req.on('error', ...) block, but still throw it from in there, then you can just use assert.strictEqual() in here.
Should set req.socket._hadError to true before emit the error event.
ba8e1b7 to
c4b1a86
Compare
|
@cjihrig done! |
|
@mscdex need to restart ci task. |
|
Any idea on the semver-iness of this change? I'd prefer it not to be major but it may have to be. Thoughts? |
|
I'd prefer patch I think. |
|
@jasnell I think this change is a bugfix. |
|
Works for me |
|
Landed in 620ba41. |
req.socket._hadError should be set before emitting the error event. PR-URL: #14659 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Should set req.socket._hadError to true before emit the error event.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http