http: don't double-fire the req error event #14333
Conversation
| process.on('uncaughtException', common.mustCall((err) => { | ||
| uncaughtExceptionCount++; | ||
| console.log(' -> uncaughtException fire, count: %s', uncaughtExceptionCount); | ||
| assert(count === 1, 'http request error event should only fire once'); |
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).
| const URL = url.URL; | ||
| const testPath = '/foo?bar'; | ||
|
|
||
| const u = `http://${common.localhostIPv4}:40404${testPath}`; |
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.
fengmk2
force-pushed
the
http-client-req-error-dont-double-fire
branch
from
03aa455
to
5e9c272
Compare
| const http = require('http'); | ||
|
|
||
| // not exists host | ||
| const host = '*'.repeat(256); |
There was a problem hiding this comment.
@mscdex I use not exists host instead.
|
|
||
| 'use strict'; | ||
| const common = require('../common'); | ||
| const assert = require('assert'); |
fengmk2
force-pushed
the
http-client-req-error-dont-double-fire
branch
from
5e9c272
to
934cb88
Compare
| port: 1, | ||
| host, | ||
| }); | ||
| let count = 0; |
There was a problem hiding this comment.
These count variables and console.log()s are no longer needed either IMHO.
fengmk2
force-pushed
the
http-client-req-error-dont-double-fire
branch
from
934cb88
to
ba8e1b7
Compare
|
@mscdex @dead-horse Thanks for the quickly review, all changed. |
| @@ -0,0 +1,35 @@ | |||
| // Copyright Joyent, Inc. and other Node contributors. | |||
There was a problem hiding this comment.
I don't think you have to include these first 21 lines in new files.
|
|
||
| // not exists host | ||
| const host = '*'.repeat(256); | ||
| const req = http.get({ |
There was a problem hiding this comment.
const req = http.get({ host }); should fit comfortably on one line.
| throw new Error('mock unexpected code error'); | ||
| })); | ||
|
|
||
| process.on('uncaughtException', common.mustCall()); |
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.
fengmk2
force-pushed
the
http-client-req-error-dont-double-fire
branch
from
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