http: client aborted should be boolean. by ronag · Pull Request #20230 · nodejs/node

ronag · 2018-04-23T16:09:21Z

This is a semver-major.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt · 2018-04-23T16:48:55Z

This should go before ### response.addTrailers(headers) ABC-wise.

vsemozhetbyt · 2018-04-23T16:49:18Z

Properties need explicit types documented, in this case:
* {boolean}

vsemozhetbyt · 2018-04-23T16:49:58Z

Is it a typo or confusing API? response.aborted means request is aborted?

vsemozhetbyt · 2018-04-23T16:51:22Z

This should go before #### response.addTrailers(headers) ABC-wise.
Heading level should be increased from ### to ####.

vsemozhetbyt · 2018-04-23T16:51:45Z

Properties need explicit types documented, in this case:
* {boolean}

vsemozhetbyt · 2018-04-23T16:52:09Z

Is it a typo or confusing API? response.aborted means request is aborted?

mscdex · 2018-04-23T17:25:32Z

This should not have passed a make test. If it did, we need to update Makefile to fix the conflict detection.

That was just me in a hurry. Apologies.

lpinca · 2018-04-26T05:53:28Z

Why does this depend on #20094? #20094 works on http.IncomingMessage while this on http.ClientRequest. Can't we simply make the ClientRequestaborted property a boolean here?

ronag · 2018-04-26T07:20:27Z

@lpinca because of the test

lpinca · 2018-04-26T07:25:08Z

Ok, I'm pretty sure we are already testing the aborted property on ClientRequest so maybe change the existing tests and keep the others for IncomingMessage ?

lpinca · 2018-04-26T07:34:50Z

Like test/parallel/test-http-abort-stream-end.js seems a perfect candidate for this.

ronag · 2018-04-26T07:55:35Z

@lpinca fixed

lpinca · 2018-04-26T08:00:55Z

This can be removed, and the assertion above changed to assert.strictEqual(req.aborted, false);

lpinca · 2018-04-26T08:13:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14519/

vsemozhetbyt · 2018-05-03T22:52:45Z

@ronag Can you rebase to resolve conflicts?

It seems we need two reviews from the @nodejs/tsc as this is semver-major.

cjihrig · 2018-05-03T23:05:00Z

Just out of curiosity, why is this change necessary/desirable?

ronag · 2018-05-04T09:31:41Z

@cjihrig: performance and consistency

vsemozhetbyt · 2018-05-07T00:54:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14689/

vsemozhetbyt · 2018-05-08T00:18:06Z

Rerun one failed task: https://ci.nodejs.org/job/node-test-commit-freebsd/17576/ (green).

vsemozhetbyt · 2018-05-08T00:19:18Z

@nodejs/tsc Please review as this is semver-major.

vsemozhetbyt · 2018-05-13T15:31:50Z

@nodejs/tsc, how can we proceed with this PR?

mcollina · 2018-05-13T16:01:20Z

@vsemozhetbyt this needs two @nodejs/tsc LGTMs to land.

@ronag can you please elaborate on performance and consistency? Why does this impact performance? Also, we are actually removing a functionality here.

This requires a CITGM to land.

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1406/
CITGM (this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1407/

apapirovski · 2018-05-13T16:03:58Z

Calling Date.now is pretty damn slow so I would assume that's the performance part here. As for consistency, I think we added this property elsewhere where it's only a boolean (for the performance reasons). Personally, I'm +1 on this change.

addaleax · 2018-05-13T16:04:28Z

Can you add a changes: block in the metadata for this property? (e.g. what we have for createServer() further below in the file)

lpinca · 2018-05-13T16:30:01Z

@mcollina for consistency see #20094.

mcollina

LGTM

vsemozhetbyt · 2018-05-13T18:19:56Z

+changes:
+ - version: REPLACEME
+ pr-url: https://github.com/nodejs/node/pull/20230
+ description: The aborted property is no longer a timestamp number.


Nit: aborted -> `aborted`?

vsemozhetbyt · 2018-05-14T16:18:31Z

We have 2 approvements from the TSC, so this is the final CI:
https://ci.nodejs.org/job/node-test-pull-request/14864/

I will land soon if there are no objections and CI is green (I can fix the doc nit on landing).

PR-URL: #20230 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

vsemozhetbyt · 2018-05-14T20:09:40Z

Landed in 4b00c4f
Thank you!

Nocks behavior around events and errors relating to aborting and ending requests has not lined up with Node for while. I dug into an investigation of how it differs, if there were git commits/pull-requests explaining why they differ, and trying to answer the question of should Nock change to line up better. The single largest deviation were the errors emitted around aborting or already aborted requests. Emitting an error on abort was added to fix issue nock#439. The conversation talks about how Node emits an error post abort. > ... in practice the `http` module does emit an `ECONNRESET` error after aborting. ... - Jan 7, 2016 However, this is only true between the 'socket' and 'response' events. Otherwise calling abort does not emit an error of any kind. Determining exactly what Nock should do is a bit of moving target. As Node's internals of HTTP, Net, Stream, and micro task timing have changed a lot since iojs/v4, when Nock added these errors, and v13, current at this time. My conclusion is that none of Nock's deviations from Node's _documented_ behavior are user features that should be maintained. If anything, bringing Nock closer to Node better fulfills original requests. nock#282 The entire `test_abort.js` file was scraped and rewritten by writing assertions against Node behavior, without Nock first. Then adding Nock into each test to ensure events and errors were correct. BREAKING CHANGE: Calling `request.abort()`: - Use to always emit a 'socket hang up' error. Now only emits the error if `abort` is called between the 'socket' and 'response' events. - The emitted 'abort' event now happens on `nextTick`. - The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object. - `request.aborted` attribute is set to `true` instead of a timestamp. Changed in Node v11.0 nodejs/node#20230 Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error. However, writing to a request that is already finished (ended) will emit a 'write after end' error. Playback of a mocked responses will now never happen until the 'socket' event is emitted. The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created. This means in the following code the Scope will never be done because at least one tick needs to happen before any matched Interceptor is considered consumed. ```js const scope = nock(...).get('/').reply() const req = http.get(...) scope.done() ```

Nocks behavior around events and errors relating to aborting and ending requests has not lined up with Node for while. I dug into an investigation of how it differs, if there were git commits/pull-requests explaining why they differ, and trying to answer the question of should Nock change to line up better. The single largest deviation were the errors emitted around aborting or already aborted requests. Emitting an error on abort was added to fix issue #439. The conversation talks about how Node emits an error post abort. > ... in practice the `http` module does emit an `ECONNRESET` error after aborting. ... - Jan 7, 2016 However, this is only true between the 'socket' and 'response' events. Otherwise calling abort does not emit an error of any kind. Determining exactly what Nock should do is a bit of moving target. As Node's internals of HTTP, Net, Stream, and micro task timing have changed a lot since iojs/v4, when Nock added these errors, and v13, current at this time. My conclusion is that none of Nock's deviations from Node's _documented_ behavior are user features that should be maintained. If anything, bringing Nock closer to Node better fulfills original requests. #282 The entire `test_abort.js` file was scraped and rewritten by writing assertions against Node behavior, without Nock first. Then adding Nock into each test to ensure events and errors were correct. BREAKING CHANGE: Calling `request.abort()`: - Use to always emit a 'socket hang up' error. Now only emits the error if `abort` is called between the 'socket' and 'response' events. - The emitted 'abort' event now happens on `nextTick`. - The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object. - `request.aborted` attribute is set to `true` instead of a timestamp. Changed in Node v11.0 nodejs/node#20230 Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error. However, writing to a request that is already finished (ended) will emit a 'write after end' error. Playback of a mocked responses will now never happen until the 'socket' event is emitted. The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created. This means in the following code the Scope will never be done because at least one tick needs to happen before any matched Interceptor is considered consumed. ```js const scope = nock(...).get('/').reply() const req = http.get(...) scope.done() ```

…k#1960) Nocks behavior around events and errors relating to aborting and ending requests has not lined up with Node for while. I dug into an investigation of how it differs, if there were git commits/pull-requests explaining why they differ, and trying to answer the question of should Nock change to line up better. The single largest deviation were the errors emitted around aborting or already aborted requests. Emitting an error on abort was added to fix issue nock#439. The conversation talks about how Node emits an error post abort. > ... in practice the `http` module does emit an `ECONNRESET` error after aborting. ... - Jan 7, 2016 However, this is only true between the 'socket' and 'response' events. Otherwise calling abort does not emit an error of any kind. Determining exactly what Nock should do is a bit of moving target. As Node's internals of HTTP, Net, Stream, and micro task timing have changed a lot since iojs/v4, when Nock added these errors, and v13, current at this time. My conclusion is that none of Nock's deviations from Node's _documented_ behavior are user features that should be maintained. If anything, bringing Nock closer to Node better fulfills original requests. nock#282 The entire `test_abort.js` file was scraped and rewritten by writing assertions against Node behavior, without Nock first. Then adding Nock into each test to ensure events and errors were correct. BREAKING CHANGE: Calling `request.abort()`: - Use to always emit a 'socket hang up' error. Now only emits the error if `abort` is called between the 'socket' and 'response' events. - The emitted 'abort' event now happens on `nextTick`. - The socket is now only destroyed if the 'socket' event has fired, and now emits a 'close' event on `nextTick` that propagates though the request object. - `request.aborted` attribute is set to `true` instead of a timestamp. Changed in Node v11.0 nodejs/node#20230 Calling `write`, `end`, or `flushHeaders` on an aborted request no longer emits an error. However, writing to a request that is already finished (ended) will emit a 'write after end' error. Playback of a mocked responses will now never happen until the 'socket' event is emitted. The 'socket' event is still artificially set to emit on `nextTick` when a ClientRequest is created. This means in the following code the Scope will never be done because at least one tick needs to happen before any matched Interceptor is considered consumed. ```js const scope = nock(...).get('/').reply() const req = http.get(...) scope.done() ```

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 23, 2018

ronag changed the title ~~Http client boolean aborted~~http: client aborted should be boolean.Apr 23, 2018

vsemozhetbyt added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels Apr 23, 2018

vsemozhetbyt reviewed Apr 23, 2018
View reviewed changes

ronag force-pushed the http-client-boolean-aborted branch 3 times, most recently from f854fb7 to 01820ddCompare April 23, 2018 17:00

ronag mentioned this pull request Apr 23, 2018
http request fixes #20075
Closed
3 tasks

mscdex reviewed Apr 23, 2018
View reviewed changes

ronag force-pushed the http-client-boolean-aborted branch from 01820dd to 3711eb5Compare April 23, 2018 17:34

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 26, 2018

ronag force-pushed the http-client-boolean-aborted branch from 3711eb5 to 69d16fcCompare April 26, 2018 07:54

lpinca reviewed Apr 26, 2018
View reviewed changes

ronag force-pushed the http-client-boolean-aborted branch from 69d16fc to 0d5ccdeCompare April 26, 2018 08:10

lpinca approved these changes Apr 26, 2018
View reviewed changes

lpinca removed http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 26, 2018

ronag force-pushed the http-client-boolean-aborted branch from 0d5ccde to 793c2c3Compare May 4, 2018 09:31

addaleax reviewed May 13, 2018
View reviewed changes

mcollina approved these changes May 13, 2018
View reviewed changes

http: client aborted should be boolean.
0995002

ronag force-pushed the http-client-boolean-aborted branch from 793c2c3 to 0995002Compare May 13, 2018 17:19

vsemozhetbyt reviewed May 13, 2018
View reviewed changes

jasnell approved these changes May 14, 2018
View reviewed changes

vsemozhetbyt pushed a commit that referenced this pull request May 14, 2018
http: make client .aborted boolean
4b00c4f
PR-URL: #20230 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

vsemozhetbyt closed this May 14, 2018

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label May 14, 2018

mastermatt mentioned this pull request Mar 24, 2020
fix(router): bring behavior of abort() inline with native Node nock/nock#1960
Merged

Uh oh!

Uh oh!

Conversation

ronag commented Apr 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexApr 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronagApr 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Apr 26, 2018

Uh oh!

ronag commented Apr 26, 2018

Uh oh!

lpinca commented Apr 26, 2018

Uh oh!

lpinca commented Apr 26, 2018

Uh oh!

ronag commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Apr 26, 2018

Uh oh!

vsemozhetbyt commented May 3, 2018

Uh oh!

cjihrig commented May 3, 2018

Uh oh!

ronag commented May 4, 2018

Uh oh!

vsemozhetbyt commented May 7, 2018

Uh oh!

vsemozhetbyt commented May 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented May 8, 2018

Uh oh!

vsemozhetbyt commented May 13, 2018

Uh oh!

mcollina commented May 13, 2018

Uh oh!

apapirovski commented May 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented May 13, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented May 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented May 14, 2018

Uh oh!

Reviewers

ronag commented Apr 23, 2018•
edited
Loading

mscdexApr 23, 2018•
edited
Loading

ronagApr 23, 2018•
edited
Loading

vsemozhetbyt commented May 8, 2018•
edited
Loading

lpinca commented May 13, 2018•
edited
Loading

vsemozhetbyt commented May 14, 2018•
edited
Loading