http: emit 'aborted' event for ClientRequest #15270
Conversation
|
If we are going introduce this new event, I would think it should only be emitted if the user calls |
|
@mscdex: It's not a new event see (https://nodejs.org/api/http.html#http_event_aborted). It's a bug. See, #15259 There is already a working event for |
lib/_http_client.js
Outdated
| req.emit('close'); | ||
| if (req.res && req.res.readable) { | ||
| // Socket closed before we emitted 'end' below. | ||
| req.res.emit('aborted'); | ||
| req.aborted = Date.now(); |
There was a problem hiding this comment.
I think this property is currently only set when calling req.abort() so this looks like a semver-major change.
|
@lpinca: Removed sem major breaking changes. |
lib/_http_client.js
Outdated
| req.emit('error', createHangUpError()); | ||
| } | ||
|
|
||
| req.emit('close'); |
There was a problem hiding this comment.
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.
lib/_http_client.js
Outdated
| @@ -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()); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think aborted should at least be emitted after error in case someone removes their error listener after aborted? Anyone else?
There was a problem hiding this comment.
I'm 👎 in not emitting an error here.
There was a problem hiding this comment.
@mcollina: aborted after or before error?
lib/_http_client.js
Outdated
| if (req.res && req.res.readable) { | ||
| // Socket closed before we emitted 'end' below. | ||
| req.emit('aborted'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, from an api user's perspective the response could conceptually be part of the request (think h2)?
There was a problem hiding this comment.
I think both behaviours make sense. I would prefer the h2 way just to avoid confusion.
|
We need to test this with the pump module: https://www.npmjs.com/package/pump. This needs unit tests as well. |
|
@ronag would you mind adding a unit test for this? |
|
@mcollina: I haven't quite figured out the unit test thing for node yet. Maybe over the weekend. |
|
We might also consider emitting a ECONNRESET for the response case as well and then deprecate the aborted event (i.e. remove it from the docs)? |
|
@ronag, one thing at a time. Deprecating something is way more sensitive than fixing a bug. |
|
@mcollina: I'll make it a separate issue |
|
Competing PR #15471 |
|
Added the semver-minor label as this is technically a new event. The |
|
@BridgeAR: Great! However, we still need to move the |
|
Ok, I guess you want to have this reopened. |
|
I see you just opened a new PR in the very same moment I reopened this PR. That was timing ^^. |
Not sure if this is entirely correct. I think we need a check for whether the request completed normally before emitting
aborted.It also needs a test case.