fix(ClientRequest): Use native abort and destroy

[mastermatt]

• Stop monkey patching abort on the overridden ClientRequest.
• Stop prematurely associating the socket with the request.
• Prefer destroy instead of emitting an error when appropriate.

---

The Catalyst

Overall I think this change is great because it brings the overridden ClientRequest even more inline with native Node.

There were two external changes that prompted this update: First ClientRequest.abort was deprecated in Node 14.1 in favor of ClientRequest.destroy. And second Got v11 began handling only the first error emitted by a ClientRequest, which caused subsequent errors to bubble up as unhandled when using the client lib.
A discussion on Got's change as well as the direction of Node's changes with two Node contributors can be found here sindresorhus/got#1224.

As a rule, Nock tries to not make exceptions for client libs, however, Got's reasoning is founded in the expected behavior of the stream.Writable "error" event:

After 'error', no further events other than 'close' should be emitted (including 'error' events).

Plus, since Nock uses Got for its own tests, it is convenient when they play well together.

The changes in Node are being driven by the desire to better align the interfaces and usages of Streams. ClientRequest is a Writeable after all. Note this comment:

... starting Node.js 14.0.0 the recommended approach [is] to req.destroy(...) and not req.emit('error', ...). Streams that have errored should be destroyed.

The Issue

The current behavior of Nock is to emit an error on the request if the second matching phase finds no match and the Scope does no allow unmocked requests. This is the "Nock: No match for request ..." error. This step happens to be between the 'socket' and 'response' events on the ClientRequest. In native Node, emitting an error on the request between those two events causes a "socket hang up" error to be emitted as well, which Nock faithfully replicates. However, it does mean that a "No match for request" error results in two errors being emitted on the request.

The ever-moving target of Node

The tl;dr on how to fix this is to have Nock call req.destroy with the "No match for request" error. destroy "cancels" the request and ignores subsequent calls.

However, Node 14 changed the internal behavior of destroy. Previously it was defined on OutgoingMessage and simply proxied to destroy on the message's socket. 14 added destroy as a method of ClientRequest and consumed most of the responsibilities of abort. To more align OutgoingMessage and ClientRequest.

Because Nock has been monkey patching abort, my first attempt at this included monkey patching destroy too and sniffing the Node major version. It was gross.
Then I realized the methods weren't doing anything too fancy outside of handling the state of a socket on a request instance. So I went the direction of removing the abort monkey patch and alining the Socket behavior more with native Node.
This required that we stop attaching the Nock socket on the request right away, instead waiting for a next tick, when we artificially fire a 'socket' event. It also meant the "socket hang up" error needed to be moved to an event listener on the socket's 'close' event. Which is how the native ClientRequest handles it. It's probably worth noting that _http_client.js does quite a bit more in listeners of the socket's 'close' and 'end' events. But Nock doesn't currently handle the flows and it seemed like overkill for this PR.

This change was tested on Node 10, 12, & 14 with Got 10.5 & 11.1.

[mastermatt]

This should fix #1992, but I want to do a bit of a write up in the description before I mark it as ready for review.
Not going to happen tonight.

[mastermatt]

That description is more long-winded than I intended, but I used it to debrief myself.
RFR.

[mastermatt]

@nock/maintainers

[paulmelnikow]

This looks good to me! Thanks for the long narrative; I think it makes it possible to follow where you went, and a good place to start the discussion if it causes unanticipated issues.

Only question I have is, from the perspective of the mock surface, do you think this should be considered a breaking change?

[mastermatt]

Only question I have is, from the perspective of the mock surface, do you think this should be considered a breaking change?

I'm not sure. I think it should go out on the beta. And not backport unless someone comes up with a really compelling reason.
This changes reduces the possible number of multiple errors emitted on a request. Having more than one as always been an undocumented, unintended action of Nock. Debatably a bug.

My recommendation is to not include this change in the breaking changes list of the next major. I think it's too subtle and will confuse the already decent size list of proper breaking changes.

[paulmelnikow]

I'm not sure. I think it should go out on the beta. And not backport unless someone comes up with a really compelling reason.

Ah, right, I'd forgotten that we hadn't released yet! Yea, I think that works out well.

[github-actions]

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

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀