Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: use autoDestroy: true in incoming message #33035

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,25 +430,13 @@ function socketCloseListener() {
req.destroyed = true;
if (res) {
// Socket closed before we emitted 'end' below.
// TOOD(ronag): res.destroy(err)
if (!res.complete) {
res.aborted = true;
res.emit('aborted');
if (res.listenerCount('error') > 0) {
res.emit('error', connResetException('aborted'));
}
res.destroy(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 @@ -697,7 +685,6 @@ 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 @@ -752,13 +739,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');
}

dnlup marked this conversation as resolved.
Show resolved Hide resolved
}

function tickOnSocket(req, socket) {
Expand Down
47 changes: 35 additions & 12 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 Stream = require('stream');
const { Readable, finished } = require('stream');

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

Stream.Readable.call(this, { autoDestroy: false, ...streamOptions });
Readable.call(this, 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, Stream.Readable.prototype);
ObjectSetPrototypeOf(IncomingMessage, Stream.Readable);
ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype);
ObjectSetPrototypeOf(IncomingMessage, Readable);

ObjectDefineProperty(IncomingMessage.prototype, 'connection', {
get: function() {
Expand Down Expand Up @@ -160,18 +160,30 @@ 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(error) {
// TODO(ronag): Implement in terms of _destroy
this.destroyed = true;
if (this.socket)
this.socket.destroy(error);
return this;
};
IncomingMessage.prototype._destroy = function _destroy(err, cb) {
dnlup marked this conversation as resolved.
Show resolved Hide resolved
if (!this.readableEnded || !this.complete) {
dnlup marked this conversation as resolved.
Show resolved Hide resolved
this.aborted = true;
this.emit('aborted');
}

// If aborted and the underlying socket 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) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
this.socket.destroy(err);
dnlup marked this conversation as resolved.
Show resolved Hide resolved
ronag marked this conversation as resolved.
Show resolved Hide resolved
const cleanup = finished(this.socket, (e) => {
cleanup();
onError(this, cb, e || err);
});
} else {
onError(this, cb, err);
}
dnlup marked this conversation as resolved.
Show resolved Hide resolved
};

ronag marked this conversation as resolved.
Show resolved Hide resolved
IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
function _addHeaderLines(headers, n) {
Expand Down Expand Up @@ -349,6 +361,17 @@ IncomingMessage.prototype._dump = function _dump() {
}
};

function onError(instance, cb, error) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
dnlup marked this conversation as resolved.
Show resolved Hide resolved
// This is to keep backward compatible behavior.
// An error is emitted only if there are listeners attached to
// the event.
if (instance.listenerCount('error') > 0) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
cb(error);
} else {
cb();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this condition is too permissive. In the previous implementation, IncomingMessage doesn't propagate only abort errors if error listener count === 0. After latest changes all errors don't propagated if error listener count === 0.

Suggested change
if (instance.listenerCount('error') > 0) {
cb(error);
} else {
cb();
}
if (instance.listenerCount('error') === 0 && error.code === 'ECONNRESET' && error.message === 'aborted') {
cb();
} else {
cb(error);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks, @wa-Nadoo . I intended to keep the same behavior, so I should implement the stricter check that you suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked again, the check wasn't as strict as you suggested in the branches I modified:

Can you point me to it? Maybe I've missed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wa-Nadoo is correct.

Copy link
Member

@ronag ronag Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IncomingMessage.destroy(new Error()) without an error listener should be unhandled exception

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got confused, the check is indeed in functions that should be called only when aborting if I am not mistaken. I'll fix this and add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this. Again, thanks @wa-Nadoo and @ronag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I need to reopen this. I don't think that the behavior was as you explained in the previous implementation. In fact, in http_server and _http_client, the points where an error is emitted are in the abort logic, and only if there are listeners attached to the event. That, and the fact the IncomingMessage wasn't implementing a standard destroy (therefore, it wasn't itself emitting an error event), made it emit an error event only in case of abort if there were event handlers.

Something that I think proves this is the fact that in _http_client, the error event of res is no bubbled up to req. The same goes for _http_server. I think maybe this example explains this: https://github.com/nodejs/node/pull/33035#discussion_r543109670.`

I added some tests, ptal.

}

module.exports = {
IncomingMessage,
readStart,
Expand Down
14 changes: 1 addition & 13 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -575,14 +575,7 @@ function socketOnClose(socket, state) {
function abortIncoming(incoming) {
while (incoming.length) {
const req = incoming.shift();
// 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');
req.destroy(connResetException('aborted'));
}
// Abort socket._httpMessage ?
}
Expand Down Expand Up @@ -741,14 +734,9 @@ 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