http: preserve socket errors as response error causes · nodejs/node@ab93a3d · GitHub
Skip to content

Commit ab93a3d

Browse files
XadillaXaduh95
authored andcommitted
http: preserve socket errors as response error causes
Keep the original socket error as the cause of ECONNRESET errors emitted when an HTTP response closes before completion. Preserve the existing aborted message, error code, and event order. Leave cause absent when there is no underlying error. Allow ConnResetException to accept Error options, document the behavior, and cover TLS record errors, TCP resets, explicit destruction, and premature closure without a socket error. This follows investigation of #66001 and addresses lost error context. The original TLS decryption failure remains unresolved. Refs: #66001 Signed-off-by: XadillaX <i@2333.moe> PR-URL: #66061 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent b901b97 commit ab93a3d

5 files changed

Lines changed: 181 additions & 4 deletions

File tree

‎doc/api/http.md‎

Lines changed: 9 additions & 1 deletion

‎lib/_http_client.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,8 @@ function socketCloseListener() {
733733
if (res) {
734734
// Socket closed before we emitted 'end' below.
735735
if (!res.complete) {
736-
res.destroy(new ConnResetException('aborted'));
736+
res.destroy(new ConnResetException('aborted',
737+
req[kError] ? { cause: req[kError] } : undefined));
737738
}
738739
req._closed = true;
739740
req.emit('close');
@@ -774,6 +775,9 @@ function socketErrorListener(err) {
774775
// and we need to make sure we don't double-fire the error event.
775776
socket._hadError = true;
776777
emitErrorEvent(req, err);
778+
// Preserve the socket error for an incomplete response, including TLS
779+
// errors that are emitted without setting socket.errored.
780+
req[kError] ||= err;
777781
}
778782

779783
const parser = socket.parser;

‎lib/internal/errors.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -845,8 +845,8 @@ class DNSException extends Error {
845845
}
846846

847847
class ConnResetException extends Error {
848-
constructor(msg) {
849-
super(msg);
848+
constructor(msg, options) {
849+
super(msg, options);
850850
this.code = 'ECONNRESET';
851851
}
852852

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
const { Writable, pipeline } = require('stream');
7+
8+
for (const method of ['emit', 'emitAndDestroy', 'socketDestroy', 'requestDestroy', 'reset', 'close']) {
9+
let error = method === 'close' || method === 'reset' ? null : new Error('Socket failure');
10+
let serverSocket;
11+
const server = http.createServer(common.mustCall((req, res) => {
12+
serverSocket = req.socket;
13+
res.writeHead(200, { 'Content-Length': 100 });
14+
res.write('partial body');
15+
}));
16+
17+
server.listen(0, common.mustCall(() => {
18+
const req = http.get({
19+
port: server.address().port,
20+
agent: false,
21+
}, common.mustCall((res) => {
22+
res.on('aborted', common.mustCall());
23+
res.on('close', common.mustCall());
24+
pipeline(res, new Writable({
25+
write(chunk, encoding, callback) {
26+
callback();
27+
},
28+
}), common.mustCall((err) => {
29+
assert.strictEqual(res.complete, false);
30+
assert.strictEqual(res.aborted, true);
31+
assert.strictEqual(err.code, 'ECONNRESET');
32+
assert.strictEqual(err.message, 'aborted');
33+
assert.strictEqual(res.errored, err);
34+
if (error) {
35+
assert.notStrictEqual(err, error);
36+
assert.strictEqual(err.cause, error);
37+
assert.deepStrictEqual(Object.getOwnPropertyDescriptor(err, 'cause'), {
38+
value: error,
39+
writable: true,
40+
enumerable: false,
41+
configurable: true,
42+
});
43+
} else {
44+
assert.strictEqual(Object.hasOwn(err, 'cause'), false);
45+
}
46+
server.close(common.mustCall());
47+
}));
48+
49+
switch (method) {
50+
case 'emit':
51+
case 'emitAndDestroy':
52+
// TLSSocket can emit an error without setting socket.errored.
53+
req.socket.emit('error', error);
54+
break;
55+
case 'socketDestroy':
56+
req.socket.destroy(error);
57+
break;
58+
case 'requestDestroy':
59+
req.destroy(error);
60+
break;
61+
case 'reset':
62+
serverSocket.resetAndDestroy();
63+
break;
64+
case 'close':
65+
req.socket.destroy();
66+
break;
67+
}
68+
}));
69+
req.on('error', method !== 'close' ? common.mustCall((err) => {
70+
if (method === 'reset') {
71+
assert.strictEqual(err.code, 'ECONNRESET');
72+
assert.strictEqual(err.syscall, 'read');
73+
error = err;
74+
} else {
75+
assert.strictEqual(err, error);
76+
}
77+
if (method === 'emitAndDestroy')
78+
req.destroy();
79+
}) : common.mustNotCall());
80+
req.on('close', common.mustCall());
81+
}));
82+
}
Lines changed: 83 additions & 0 deletions

0 commit comments

Comments
 (0)