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.

Sign up for GitHub

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

http2: add missing error handlers #19986

Closed
wants to merge 2 commits into from
Closed

http2: add missing error handlers #19986

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac893b6
http2: add missing error handlers
ryzokuken Apr 12, 2018
cd4af2a
Incorporate feedback
ryzokuken Apr 14, 2018
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
7 changes: 7 additions & 0 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ function emit(self, ...args) {
self.emit(...args);
}

const noop = () => {};

// Called when a new block of headers has been received for a given
// stream. The stream may or may not be new. If the stream is new,
// create the associated Http2Stream instance and emit the 'stream'
Expand Down Expand Up @@ -868,6 +870,9 @@ class Http2Session extends EventEmitter {

if (!socket._handle || !socket._handle._externalStream) {
socket = new StreamWrap(socket);
socket.on('error', (err) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the proper error handling would look like here. I think it might need to do more than emit and it should also be aware of whether the socketOnError already fired previously.

Maybe something like this would be more accurate:

const wrappedSocket = socket;
socket = new StreamWrap(wrappedSocket);
socket.on('error', socketOnError.bind(wrappedSocket));

This would avoid the JSStreamWrap propagating an already handled error from the underlying socket and emitting on the session twice.

/cc @jasnell @mcollina @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

I also think this might need to handle socket.on('close') in a similar manner. So something like:

socket.on('close', socketOnClose.bind(wrappedSocket));

Copy link
Member

Choose a reason for hiding this comment

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

Proper error handling here is a simple .on('error', () => {}) as the underlying stream is already handled before hitting this switch

Copy link
Member

Choose a reason for hiding this comment

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

/cc @ryzokuken

Copy link
Member

@apapirovski apapirovski Apr 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Proper error handling here is a simple .on('error', () => {}) as the underlying stream is already handled before hitting this switch

That presumes that the wrap can't error itself...

(Which it can for example on line 56 in the JSStreamWrap itself, not to mention the fact that it inherits from Socket so there are likely other errors possible too.)

Copy link
Member

Choose a reason for hiding this comment

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

I would like input from @mcollina and @jasnell if possible. I think the following is correct:

const wrappedSocket = socket;
socket = new StreamWrap(wrappedSocket);
socket.on('error', socketOnError.bind(wrappedSocket));
socket.on('close', socketOnClose.bind(wrappedSocket));

Copy link
Member

Choose a reason for hiding this comment

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

that will trigger the error twice, as the wrappedSocket already triggers socketOnClose and socketOnError.

I think it would be fine to trigger an error if the wrappedSocket is in objectMode immediately as that seems to be the only case where the streamwrap emits a non-forwarded error? And then socket.on('error', () => {}) the rest, as they are already handled

Copy link
Member

@mafintosh mafintosh Apr 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the http2 code it seems ok to call socketOnError more than once as that simply calls destroy which should guard against that. The onClose method has a bit more going on so can't tell if that is safe to do there, but i don't think we need to forward that in either case?

so

const wrappedSocket = socket;
socket = new StreamWrap(wrappedSocket);
socket.on('error', socketOnError.bind(wrappedSocket));

?

Copy link
Member

Choose a reason for hiding this comment

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

that will trigger the error twice, as the wrappedSocket already triggers socketOnClose and socketOnError.

It will just bail because socketOnError removes the session assignment in a sync call. I'm also not sure that the error in objectMode is the only one. The wrap is a socket in itself, it can throw other related errors.

Copy link
Member

Choose a reason for hiding this comment

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

And I think you're probably right re: leaving out close listener. So the thing posted above seems good to me.

this.emit('error', err);
});
}

// No validation is performed on the input parameters because this
Expand Down Expand Up @@ -2518,6 +2523,8 @@ function connectionListener(socket) {
// Let event handler deal with the socket
debug(`Unknown protocol from ${socket.remoteAddress}:${socket.remotePort}`);
if (!this.emit('unknownProtocol', socket)) {
socket.on('error', noop);

// We don't know what to do, so let's just tell the other side what's
// going on in a format that they *might* understand.
socket.end('HTTP/1.0 403 Forbidden\r\n' +
Expand Down