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 c784f15

Browse filesBrowse files
dnlupMylesBorins
dnlup
authored and
MylesBorins
committed
Revert "http: use autoDestroy: true in incoming message"
This reverts commits: * 55e83cb * 6120028 * 70eaf55 * 5ae9690 * f20a88f * a6bf74e * 8154e47 Because of the breaking change in the order of emitting the `close` event in `IncomingMessage` described in: #33035 (comment) PR-URL: #36647 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent ccd900f commit c784f15
Copy full SHA for c784f15

File tree

Expand file treeCollapse file tree

5 files changed

+43
-86
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+43
-86
lines changed
Open diff view settings
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+18-1Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,13 +430,25 @@ function socketCloseListener() {
430430
req.destroyed = true;
431431
if (res) {
432432
// Socket closed before we emitted 'end' below.
433+
// TOOD(ronag): res.destroy(err)
433434
if (!res.complete) {
434-
res.destroy(connResetException('aborted'));
435+
res.aborted = true;
436+
res.emit('aborted');
437+
if (res.listenerCount('error') > 0) {
438+
res.emit('error', connResetException('aborted'));
439+
}
435440
}
436441
req._closed = true;
437442
req.emit('close');
438443
if (!res.aborted && res.readable) {
444+
res.on('end', function() {
445+
this.destroyed = true;
446+
this.emit('close');
447+
});
439448
res.push(null);
449+
} else {
450+
res.destroyed = true;
451+
res.emit('close');
440452
}
441453
} else {
442454
if (!req.socket._hadError) {
@@ -685,6 +697,7 @@ function responseKeepAlive(req) {
685697

686698
req.destroyed = true;
687699
if (req.res) {
700+
req.res.destroyed = true;
688701
// Detach socket from IncomingMessage to avoid destroying the freed
689702
// socket in IncomingMessage.destroy().
690703
req.res.socket = null;
@@ -739,6 +752,10 @@ function requestOnPrefinish() {
739752
function emitFreeNT(req) {
740753
req._closed = true;
741754
req.emit('close');
755+
if (req.res) {
756+
req.res.emit('close');
757+
}
758+
742759
if (req.socket) {
743760
req.socket.emit('free');
744761
}
Collapse file

‎lib/_http_incoming.js‎

Copy file name to clipboardExpand all lines: lib/_http_incoming.js
+12-34Lines changed: 12 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ const {
2727
Symbol
2828
} = primordials;
2929

30-
const { Readable, finished } = require('stream');
30+
const Stream = require('stream');
3131

3232
const kHeaders = Symbol('kHeaders');
3333
const kHeadersCount = Symbol('kHeadersCount');
@@ -54,7 +54,7 @@ function IncomingMessage(socket) {
5454
};
5555
}
5656

57-
Readable.call(this, streamOptions);
57+
Stream.Readable.call(this, { autoDestroy: false, ...streamOptions });
5858

5959
this._readableState.readingMore = true;
6060

@@ -89,8 +89,8 @@ function IncomingMessage(socket) {
8989
// read by the user, so there's no point continuing to handle it.
9090
this._dumped = false;
9191
}
92-
ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype);
93-
ObjectSetPrototypeOf(IncomingMessage, Readable);
92+
ObjectSetPrototypeOf(IncomingMessage.prototype, Stream.Readable.prototype);
93+
ObjectSetPrototypeOf(IncomingMessage, Stream.Readable);
9494

9595
ObjectDefineProperty(IncomingMessage.prototype, 'connection', {
9696
get: function() {
@@ -160,31 +160,19 @@ IncomingMessage.prototype._read = function _read(n) {
160160
readStart(this.socket);
161161
};
162162

163+
163164
// It's possible that the socket will be destroyed, and removed from
164165
// any messages, before ever calling this. In that case, just skip
165166
// it, since something else is destroying this connection anyway.
166-
IncomingMessage.prototype._destroy = function _destroy(err, cb) {
167-
if (!this.readableEnded || !this.complete) {
168-
this.aborted = true;
169-
this.emit('aborted');
170-
}
171-
172-
// If aborted and the underlying socket is not already destroyed,
173-
// destroy it.
174-
// We have to check if the socket is already destroyed because finished
175-
// does not call the callback when this methdod is invoked from `_http_client`
176-
// in `test/parallel/test-http-client-spurious-aborted.js`
177-
if (this.socket && !this.socket.destroyed && this.aborted) {
178-
this.socket.destroy(err);
179-
const cleanup = finished(this.socket, (e) => {
180-
cleanup();
181-
onError(this, e || err, cb);
182-
});
183-
} else {
184-
onError(this, err, cb);
185-
}
167+
IncomingMessage.prototype.destroy = function destroy(error) {
168+
// TODO(ronag): Implement in terms of _destroy
169+
this.destroyed = true;
170+
if (this.socket)
171+
this.socket.destroy(error);
172+
return this;
186173
};
187174

175+
188176
IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
189177
function _addHeaderLines(headers, n) {
190178
if (headers && headers.length) {
@@ -361,16 +349,6 @@ IncomingMessage.prototype._dump = function _dump() {
361349
}
362350
};
363351

364-
function onError(self, error, cb) {
365-
// This is to keep backward compatible behavior.
366-
// An error is emitted only if there are listeners attached to the event.
367-
if (self.listenerCount('error') === 0) {
368-
cb();
369-
} else {
370-
cb(error);
371-
}
372-
}
373-
374352
module.exports = {
375353
IncomingMessage,
376354
readStart,
Collapse file

‎lib/_http_server.js‎

Copy file name to clipboardExpand all lines: lib/_http_server.js
+13-1Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -575,7 +575,14 @@ function socketOnClose(socket, state) {
575575
function abortIncoming(incoming) {
576576
while (incoming.length) {
577577
const req = incoming.shift();
578-
req.destroy(connResetException('aborted'));
578+
// TODO(ronag): req.destroy(err)
579+
req.aborted = true;
580+
req.destroyed = true;
581+
req.emit('aborted');
582+
if (req.listenerCount('error') > 0) {
583+
req.emit('error', connResetException('aborted'));
584+
}
585+
req.emit('close');
579586
}
580587
// Abort socket._httpMessage ?
581588
}
@@ -734,9 +741,14 @@ function clearIncoming(req) {
734741
if (parser && parser.incoming === req) {
735742
if (req.readableEnded) {
736743
parser.incoming = null;
744+
req.destroyed = true;
745+
req.emit('close');
737746
} else {
738747
req.on('end', clearIncoming);
739748
}
749+
} else {
750+
req.destroyed = true;
751+
req.emit('close');
740752
}
741753
}
742754

Collapse file

‎test/parallel/test-http-client-incomingmessage-destroy.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-client-incomingmessage-destroy.js
-25Lines changed: 0 additions & 25 deletions
This file was deleted.
Collapse file

‎test/parallel/test-http-server-incomingmessage-destroy.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http-server-incomingmessage-destroy.js
-25Lines changed: 0 additions & 25 deletions
This file was deleted.

0 commit comments

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