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

Jump to bottom

http: emit 'aborted' event for ClientRequest #15270

Closed
wants to merge 2 commits into from
Closed

http: emit 'aborted' event for ClientRequest #15270

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a634b83
http: emit 'aborted' event for ClientRequest
ronag Sep 8, 2017
102c621
http: don't emit aborted on request if has response.
ronag Sep 11, 2017
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
6 changes: 5 additions & 1 deletion lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,10 @@ function socketCloseListener() {
// NOTE: It's important to get parser here, because it could be freed by
// the `socketOnData`.
var parser = socket.parser;
req.emit('close');

if (req.res && req.res.readable) {
// Socket closed before we emitted 'end' below.
req.emit('aborted');
Copy link
Member

@apapirovski apapirovski Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this correct? This branch means there's a response coming but we haven't received its end. I don't think this qualifies for an 'aborted' event for the request (only the response).

If this is motivated by how this works in h2, it's slightly different there because you have the request & response in one (duplex stream), instead of separate like with http1.

Then again, maybe I'm severely overthinking this.

Copy link
Member Author

@ronag ronag Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Well, from an api user's perspective the response could conceptually be part of the request (think h2)?

Copy link
Member Author

@ronag ronag Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think both behaviours make sense. I would prefer the h2 way just to avoid confusion.

req.res.emit('aborted');
var res = req.res;
res.on('end', function() {
Expand All @@ -374,9 +375,12 @@ function socketCloseListener() {
// receive a response. The error needs to
// fire on the request.
req.socket._hadError = true;
req.emit('aborted');
req.emit('error', createHangUpError());
Copy link
Member Author

@ronag ronag Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure error makes sense here. But changing that would be a sem-major change. @apapirovski what consequences would be removing it in favor of aborted have, e.g. during pump?

Copy link
Member Author

@ronag ronag Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think aborted should at least be emitted after error in case someone removes their error listener after aborted? Anyone else?

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 👎 in not emitting an error here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina: aborted after or before error?

Copy link
Member

Choose a reason for hiding this comment

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

aborted after.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

}

req.emit('close');
Copy link
Member Author

@ronag ronag Sep 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this correct? I think close should be emitted last. Otherwise, error might be ignored, e.g some people do:

new Promise((resolve, reject) => req.on('close', resolve).on('error', reject))

Where the error is ignored since the promise is already resolved through close.


// Too bad. That output wasn't getting written.
// This is pretty terrible that it doesn't raise an error.
// Fixed better in v0.10
Expand Down