Skip to content

Conversation

@mastermatt
Copy link
Member

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 http: client aborted should be boolean. 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.

const scope = nock(...).get('/').reply()
const req = http.get(...)
scope.done()

Closes #1953

Copy link
Member

@paulmelnikow paulmelnikow left a 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.

@mastermatt
Copy link
Member Author

I will have another pr (maybe two) with subtle breaking changes in the near-future.
My personal preference is to not have majors released close together.
What do we think about running a beta for v13?

@paulmelnikow
Copy link
Member

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.

@mastermatt
Copy link
Member Author

My understanding is that I should cut a new branch from master then base this PR and the other ones I have coming on that branch. Should the new branch be next or beta? @gr2m ?

@gr2m
Copy link
Member

gr2m commented Mar 29, 2020

If you expect breaking changes, or to "gather" breaking changes, beta is right channel

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()
```
@mastermatt mastermatt force-pushed the 1953/fix-abort-errors-and-timing branch from dbf7aa2 to 590672d Compare March 29, 2020 19:36
@mastermatt mastermatt changed the base branch from master to beta March 29, 2020 19:37
@mastermatt
Copy link
Member Author

Ok this is now based off the beta branch and ready for 👍

Copy link
Member

@gr2m gr2m left a 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

@paulmelnikow
Copy link
Member

I'll give these tests a read by tomorrow.

'finish',
'abort',
'error',
'close',
Copy link
Member

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',
Copy link
Member

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?

// 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 => {
Copy link
Member

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".

@mastermatt mastermatt merged commit 5c1b937 into nock:beta Apr 4, 2020
@mastermatt mastermatt deleted the 1953/fix-abort-errors-and-timing branch April 4, 2020 17:45
@github-actions
Copy link

github-actions bot commented Apr 4, 2020

🎉 This PR is included in version 13.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 7, 2020
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.
atscott pushed a commit to angular/angular that referenced this pull request Sep 8, 2020
…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
atscott pushed a commit to angular/angular that referenced this pull request Sep 8, 2020
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should Nock emit errors on aborted requests

3 participants