Skip to content

Commit

Permalink
Revert "http: use autoDestroy: true in incoming message"
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
dnlup authored and MylesBorins committed Jan 5, 2021
1 parent ccd900f commit c784f15
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 86 deletions.
19 changes: 18 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,13 +430,25 @@ function socketCloseListener() {
req.destroyed = true;
if (res) {
// Socket closed before we emitted 'end' below.
// TOOD(ronag): res.destroy(err)
if (!res.complete) {
res.destroy(connResetException('aborted'));
res.aborted = true;
res.emit('aborted');
if (res.listenerCount('error') > 0) {
res.emit('error', connResetException('aborted'));
}
}
req._closed = true;
req.emit('close');
if (!res.aborted && res.readable) {
res.on('end', function() {
this.destroyed = true;
this.emit('close');
});
res.push(null);
} else {
res.destroyed = true;
res.emit('close');
}
} else {
if (!req.socket._hadError) {
Expand Down Expand Up @@ -685,6 +697,7 @@ function responseKeepAlive(req) {

req.destroyed = true;
if (req.res) {
req.res.destroyed = true;
// Detach socket from IncomingMessage to avoid destroying the freed
// socket in IncomingMessage.destroy().
req.res.socket = null;
Expand Down Expand Up @@ -739,6 +752,10 @@ function requestOnPrefinish() {
function emitFreeNT(req) {
req._closed = true;
req.emit('close');
if (req.res) {
req.res.emit('close');
}

if (req.socket) {
req.socket.emit('free');
}
Expand Down
46 changes: 12 additions & 34 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const {
Symbol
} = primordials;

const { Readable, finished } = require('stream');
const Stream = require('stream');

const kHeaders = Symbol('kHeaders');
const kHeadersCount = Symbol('kHeadersCount');
Expand All @@ -54,7 +54,7 @@ function IncomingMessage(socket) {
};
}

Readable.call(this, streamOptions);
Stream.Readable.call(this, { autoDestroy: false, ...streamOptions });

this._readableState.readingMore = true;

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

ObjectDefineProperty(IncomingMessage.prototype, 'connection', {
get: function() {
Expand Down Expand Up @@ -160,31 +160,19 @@ IncomingMessage.prototype._read = function _read(n) {
readStart(this.socket);
};


// It's possible that the socket will be destroyed, and removed from
// any messages, before ever calling this. In that case, just skip
// it, since something else is destroying this connection anyway.
IncomingMessage.prototype._destroy = function _destroy(err, cb) {
if (!this.readableEnded || !this.complete) {
this.aborted = true;
this.emit('aborted');
}

// If aborted and the underlying socket is not already destroyed,
// destroy it.
// We have to check if the socket is already destroyed because finished
// does not call the callback when this methdod is invoked from `_http_client`
// in `test/parallel/test-http-client-spurious-aborted.js`
if (this.socket && !this.socket.destroyed && this.aborted) {
this.socket.destroy(err);
const cleanup = finished(this.socket, (e) => {
cleanup();
onError(this, e || err, cb);
});
} else {
onError(this, err, cb);
}
IncomingMessage.prototype.destroy = function destroy(error) {
// TODO(ronag): Implement in terms of _destroy
this.destroyed = true;
if (this.socket)
this.socket.destroy(error);
return this;
};


IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
function _addHeaderLines(headers, n) {
if (headers && headers.length) {
Expand Down Expand Up @@ -361,16 +349,6 @@ IncomingMessage.prototype._dump = function _dump() {
}
};

function onError(self, error, cb) {
// This is to keep backward compatible behavior.
// An error is emitted only if there are listeners attached to the event.
if (self.listenerCount('error') === 0) {
cb();
} else {
cb(error);
}
}

module.exports = {
IncomingMessage,
readStart,
Expand Down
14 changes: 13 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,14 @@ function socketOnClose(socket, state) {
function abortIncoming(incoming) {
while (incoming.length) {
const req = incoming.shift();
req.destroy(connResetException('aborted'));
// TODO(ronag): req.destroy(err)
req.aborted = true;
req.destroyed = true;
req.emit('aborted');
if (req.listenerCount('error') > 0) {
req.emit('error', connResetException('aborted'));
}
req.emit('close');
}
// Abort socket._httpMessage ?
}
Expand Down Expand Up @@ -734,9 +741,14 @@ function clearIncoming(req) {
if (parser && parser.incoming === req) {
if (req.readableEnded) {
parser.incoming = null;
req.destroyed = true;
req.emit('close');
} else {
req.on('end', clearIncoming);
}
} else {
req.destroyed = true;
req.emit('close');
}
}

Expand Down
25 changes: 0 additions & 25 deletions test/parallel/test-http-client-incomingmessage-destroy.js

This file was deleted.

25 changes: 0 additions & 25 deletions test/parallel/test-http-server-incomingmessage-destroy.js

This file was deleted.

0 comments on commit c784f15

Please sign in to comment.