-
-
Notifications
You must be signed in to change notification settings - Fork 752
fix(router): bring behavior of abort() inline with native Node
#1960
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
fix(router): bring behavior of abort() inline with native Node
#1960
Conversation
paulmelnikow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for digging into this! I haven't read the changes in the tests yet, though the implementation looks good and I left a few comments.
|
I will have another pr (maybe two) with subtle breaking changes in the near-future. |
|
That sounds good to me. This PR touches so many of the internals so we should try to keep it to a few weeks at most. |
|
My understanding is that I should cut a new branch from |
|
If you expect breaking changes, or to "gather" breaking changes, |
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() ```
dbf7aa2 to
590672d
Compare
|
Ok this is now based off the |
gr2m
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I think the best would be to send out a tweet to ask people to test the new beta version, and report back if their CI run into any problems
|
I'll give these tests a read by tomorrow. |
| 'finish', | ||
| 'abort', | ||
| 'error', | ||
| 'close', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why this assertion is included?
| 'finish', | ||
| 'response', | ||
| 'abort', | ||
| 'close', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, you're asserting the order of these events. In addition to asserting that 'error' isn't included, s this a confidence check?
tests/test_abort.js
Outdated
| // The order of tests run sequentially through a ClientRequest's lifetime. | ||
| // Starting the top by aborting requests early on then aborting later and later. | ||
| describe('`ClientRequest.abort()`', () => { | ||
| it('should not emit an error when `write` is called on an aborted request', done => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if all of these tests should say e.g. "Emits the expected event sequence when write is called on an aborted request".
|
🎉 This PR is included in version 13.0.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 13.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 13.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This commit upgrades all dependencies in `aio/aio-builds-setup/scripts-js/` to latest versions and also includes all necessary code changes to ensure the tests are passing with the new dependency versions. In particular: - We ensure `nock`'s `Scope#done()` is not called before receiving a response to account for a breaking change introduced in nock/nock#1960. - The use of `nock`'s `Scope#log()` method was removed, because the method is no longer available since nock/nock#1966. See https://github.com/nock/nock#debugging for more info on debugging failed matches. See also https://github.com/nock/nock/blob/e23ba31b137ec43ac83e47395b4992d62832bde1/migration_guides/migrating_to_13.md for more info on migrating from `nock` v12 to v13.
…38736) This commit upgrades all dependencies in `aio/aio-builds-setup/scripts-js/` to latest versions and also includes all necessary code changes to ensure the tests are passing with the new dependency versions. In particular: - We ensure `nock`'s `Scope#done()` is not called before receiving a response to account for a breaking change introduced in nock/nock#1960. - The use of `nock`'s `Scope#log()` method was removed, because the method is no longer available since nock/nock#1966. See https://github.com/nock/nock#debugging for more info on debugging failed matches. See also https://github.com/nock/nock/blob/e23ba31b137ec43ac83e47395b4992d62832bde1/migration_guides/migrating_to_13.md for more info on migrating from `nock` v12 to v13. PR Close #38736
…38736) This commit upgrades all dependencies in `aio/aio-builds-setup/scripts-js/` to latest versions and also includes all necessary code changes to ensure the tests are passing with the new dependency versions. In particular: - We ensure `nock`'s `Scope#done()` is not called before receiving a response to account for a breaking change introduced in nock/nock#1960. - The use of `nock`'s `Scope#log()` method was removed, because the method is no longer available since nock/nock#1966. See https://github.com/nock/nock#debugging for more info on debugging failed matches. See also https://github.com/nock/nock/blob/e23ba31b137ec43ac83e47395b4992d62832bde1/migration_guides/migrating_to_13.md for more info on migrating from `nock` v12 to v13. PR Close #38736
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.
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.jsfile 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():abortis called between the 'socket' and 'response' events.nextTick.nextTickthat propagates though the request object.request.abortedattribute is set totrueinstead of a timestamp. Changed in Node v11.0 http: client aborted should be boolean. nodejs/node#20230Calling
write,end, orflushHeaderson 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
nextTickwhen 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.
Closes #1953