FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "http: use `autoDestroy: true` in incoming message" · nodejs/node@c784f15 · GitHub

/ node Public

Commit c784f15

Browse files
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

5 files changed

Lines changed: 43 additions & 86 deletions

File tree

‎lib/_http_client.js‎

Lines 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
}

‎lib/_http_incoming.js‎

Lines 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,

‎lib/_http_server.js‎

Lines 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

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

Lines changed: 0 additions & 25 deletions
This file was deleted.

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

Lines changed: 0 additions & 25 deletions
This file was deleted.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL