Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8a6fab0

Browse filesBrowse files
ronagronag
committed
http: emit 'error' on aborted server request
Server requests aka. IncomingMessage emits 'aborted' instead of 'error' which causes confusion when the object is used as a regular stream, i.e. if functions working on streams are passed a server request object they might not work properly unless they take this into account. Refs: nodejs/web-server-frameworks#41 PR-URL: #33172 Fixes: #28172 Refs: #28677 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent cbf2fa6 commit 8a6fab0
Copy full SHA for 8a6fab0

File tree

Expand file treeCollapse file tree

8 files changed

+121
-35
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+121
-35
lines changed
Open diff view settings
Collapse file

‎doc/api/http.md‎

Copy file name to clipboardExpand all lines: doc/api/http.md
+8-3Lines changed: 8 additions & 3 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -333,9 +333,8 @@ Until the data is consumed, the `'end'` event will not fire. Also, until
333333
the data is read it will consume memory that can eventually lead to a
334334
'process out of memory' error.
335335

336-
Unlike the `request` object, if the response closes prematurely, the
337-
`response` object does not emit an `'error'` event but instead emits the
338-
`'aborted'` event.
336+
For backward compatibility, `res` will only emit `'error'` if there is an
337+
`'error'` listener registered.
339338

340339
Node.js does not check whether Content-Length and the length of the
341340
body which has been transmitted are equal or not.
@@ -2417,6 +2416,8 @@ the following events will be emitted in the following order:
24172416
* `'data'` any number of times, on the `res` object
24182417
* (connection closed here)
24192418
* `'aborted'` on the `res` object
2419+
* `'error'` on the `res` object with an error with message
2420+
`'Error: aborted'` and code `'ECONNRESET'`.
24202421
* `'close'`
24212422
* `'close'` on the `res` object
24222423

@@ -2445,6 +2446,8 @@ events will be emitted in the following order:
24452446
* `'data'` any number of times, on the `res` object
24462447
* (`req.destroy()` called here)
24472448
* `'aborted'` on the `res` object
2449+
* `'error'` on the `res` object with an error with message
2450+
`'Error: aborted'` and code `'ECONNRESET'`.
24482451
* `'close'`
24492452
* `'close'` on the `res` object
24502453

@@ -2474,6 +2477,8 @@ events will be emitted in the following order:
24742477
* (`req.abort()` called here)
24752478
* `'abort'`
24762479
* `'aborted'` on the `res` object
2480+
* `'error'` on the `res` object with an error with message
2481+
`'Error: aborted'` and code `'ECONNRESET'`.
24772482
* `'close'`
24782483
* `'close'` on the `res` object
24792484

Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,9 +417,13 @@ function socketCloseListener() {
417417
const res = req.res;
418418
if (res) {
419419
// Socket closed before we emitted 'end' below.
420+
// TOOD(ronag): res.destroy(err)
420421
if (!res.complete) {
421422
res.aborted = true;
422423
res.emit('aborted');
424+
if (res.listenerCount('error') > 0) {
425+
res.emit('error', connResetException('aborted'));
426+
}
423427
}
424428
req.emit('close');
425429
if (!res.aborted && res.readable) {
Collapse file

‎lib/_http_server.js‎

Copy file name to clipboardExpand all lines: lib/_http_server.js
+9-1Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,16 @@ const {
5656
getOrSetAsyncId
5757
} = require('internal/async_hooks');
5858
const { IncomingMessage } = require('_http_incoming');
59+
const {
60+
connResetException,
61+
codes
62+
} = require('internal/errors');
5963
const {
6064
ERR_HTTP_HEADERS_SENT,
6165
ERR_HTTP_INVALID_STATUS_CODE,
6266
ERR_INVALID_ARG_TYPE,
6367
ERR_INVALID_CHAR
64-
} = require('internal/errors').codes;
68+
} = codes;
6569
const { validateInteger } = require('internal/validators');
6670
const Buffer = require('buffer').Buffer;
6771
const {
@@ -536,9 +540,13 @@ function socketOnClose(socket, state) {
536540
function abortIncoming(incoming) {
537541
while (incoming.length) {
538542
const req = incoming.shift();
543+
// TODO(ronag): req.destroy(err)
539544
req.aborted = true;
540545
req.destroyed = true;
541546
req.emit('aborted');
547+
if (req.listenerCount('error') > 0) {
548+
req.emit('error', connResetException('aborted'));
549+
}
542550
req.emit('close');
543551
}
544552
// Abort socket._httpMessage ?
Collapse file

‎test/parallel/test-http-abort-client.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-abort-client.js
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ server.listen(0, common.mustCall(() => {
4141
res.resume();
4242
res.on('end', common.mustNotCall());
4343
res.on('aborted', common.mustCall());
44+
res.on('error', common.expectsError({
45+
code: 'ECONNRESET'
46+
}));
4447
res.on('close', common.mustCall());
4548
res.socket.on('close', common.mustCall());
4649
}));
Collapse file

‎test/parallel/test-http-aborted.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-aborted.js
+51-16Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,58 @@ const common = require('../common');
44
const http = require('http');
55
const assert = require('assert');
66

7-
const server = http.createServer(common.mustCall(function(req, res) {
8-
req.on('aborted', common.mustCall(function() {
9-
assert.strictEqual(this.aborted, true);
10-
server.close();
7+
{
8+
const server = http.createServer(common.mustCall(function(req, res) {
9+
req.on('aborted', common.mustCall(function() {
10+
assert.strictEqual(this.aborted, true);
11+
}));
12+
req.on('error', common.mustCall(function(err) {
13+
assert.strictEqual(err.code, 'ECONNRESET');
14+
assert.strictEqual(err.message, 'aborted');
15+
server.close();
16+
}));
17+
assert.strictEqual(req.aborted, false);
18+
res.write('hello');
19+
}));
20+
21+
server.listen(0, common.mustCall(() => {
22+
const req = http.get({
23+
port: server.address().port,
24+
headers: { connection: 'keep-alive' }
25+
}, common.mustCall((res) => {
26+
res.on('aborted', common.mustCall(() => {
27+
assert.strictEqual(res.aborted, true);
28+
}));
29+
res.on('error', common.expectsError({
30+
code: 'ECONNRESET',
31+
message: 'aborted'
32+
}));
33+
req.abort();
34+
}));
35+
}));
36+
}
37+
38+
{
39+
// Don't crash if no 'error' handler on server request.
40+
41+
const server = http.createServer(common.mustCall(function(req, res) {
42+
req.on('aborted', common.mustCall(function() {
43+
assert.strictEqual(this.aborted, true);
44+
server.close();
45+
}));
46+
assert.strictEqual(req.aborted, false);
47+
res.write('hello');
1148
}));
12-
assert.strictEqual(req.aborted, false);
13-
res.write('hello');
14-
}));
1549

16-
server.listen(0, common.mustCall(() => {
17-
const req = http.get({
18-
port: server.address().port,
19-
headers: { connection: 'keep-alive' }
20-
}, common.mustCall((res) => {
21-
res.on('aborted', common.mustCall(() => {
22-
assert.strictEqual(res.aborted, true);
50+
server.listen(0, common.mustCall(() => {
51+
const req = http.get({
52+
port: server.address().port,
53+
headers: { connection: 'keep-alive' }
54+
}, common.mustCall((res) => {
55+
res.on('aborted', common.mustCall(() => {
56+
assert.strictEqual(res.aborted, true);
57+
}));
58+
req.abort();
2359
}));
24-
req.abort();
2560
}));
26-
}));
61+
}
Collapse file

‎test/parallel/test-http-client-aborted-event.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-aborted-event.js
+39-14Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,44 @@
22
const common = require('../common');
33
const http = require('http');
44

5-
let serverRes;
6-
const server = http.Server(function(req, res) {
7-
res.write('Part of my res.');
8-
serverRes = res;
9-
});
5+
{
6+
let serverRes;
7+
const server = http.Server(function(req, res) {
8+
res.write('Part of my res.');
9+
serverRes = res;
10+
});
1011

11-
server.listen(0, common.mustCall(function() {
12-
http.get({
13-
port: this.address().port,
14-
headers: { connection: 'keep-alive' }
15-
}, common.mustCall(function(res) {
16-
server.close();
17-
serverRes.destroy();
18-
res.on('aborted', common.mustCall());
12+
server.listen(0, common.mustCall(function() {
13+
http.get({
14+
port: this.address().port,
15+
headers: { connection: 'keep-alive' }
16+
}, common.mustCall(function(res) {
17+
server.close();
18+
serverRes.destroy();
19+
res.on('aborted', common.mustCall());
20+
res.on('error', common.expectsError({
21+
code: 'ECONNRESET'
22+
}));
23+
}));
1924
}));
20-
}));
25+
}
26+
27+
{
28+
// Don't crash of no 'error' handler.
29+
let serverRes;
30+
const server = http.Server(function(req, res) {
31+
res.write('Part of my res.');
32+
serverRes = res;
33+
});
34+
35+
server.listen(0, common.mustCall(function() {
36+
http.get({
37+
port: this.address().port,
38+
headers: { connection: 'keep-alive' }
39+
}, common.mustCall(function(res) {
40+
server.close();
41+
serverRes.destroy();
42+
res.on('aborted', common.mustCall());
43+
}));
44+
}));
45+
}
Collapse file

‎test/parallel/test-http-client-spurious-aborted.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-spurious-aborted.js
+4-1Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,15 +60,18 @@ function download() {
6060
res.on('end', common.mustCall(() => {
6161
reqCountdown.dec();
6262
}));
63+
res.on('error', common.mustNotCall());
6364
} else {
6465
res.on('aborted', common.mustCall(() => {
6566
aborted = true;
6667
reqCountdown.dec();
6768
writable.end();
6869
}));
70+
res.on('error', common.expectsError({
71+
code: 'ECONNRESET'
72+
}));
6973
}
7074

71-
res.on('error', common.mustNotCall());
7275
writable.on('finish', () => {
7376
assert.strictEqual(aborted, abortRequest);
7477
finishCountdown.dec();
Collapse file

‎test/parallel/test-http-outgoing-message-capture-rejection.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-outgoing-message-capture-rejection.js
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ events.captureRejections = true;
3333

3434
req.on('response', common.mustCall((res) => {
3535
res.on('aborted', common.mustCall());
36+
res.on('error', common.expectsError({
37+
code: 'ECONNRESET'
38+
}));
3639
res.resume();
3740
server.close();
3841
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.