Nock is not compatible with stream.pipeline(...)

[szmarczak]

Please look at sindresorhus/got#976 for further information.

[mastermatt]

This seems to happen because the "no match" error that is emitted, when a scope matches but not an interceptor, emits after a nextTick... (inside startPlayback())

nock/lib/intercepted_request_router.js

Lines 290 to 297 in 3fbc69d

	const err = new Error(
	`Nock: No match for request ${common.stringifyRequest(
	options,
	requestBodyString
	)}`
	)
	err.statusCode = err.status = 404
	this.emitError(err)

whereas the finish and end events are emitted immediately on the request.

nock/lib/intercepted_request_router.js

Lines 147 to 149 in 3fbc69d

	this.startPlayback()
	req.emit('finish')
	req.emit('end')

Nock tries to replicate what Node does natively, but it's a little hard in this case since none of these events are documented. (I don't even think ClientRequest ever emits an end event).
Digging through the source of _http_client.js and _http_outgoing.js, it makes sense that we delay the error event, however, it's hard to find scenarios where finish is still fired after an error event is queued.
It might be possible if something attempts to write to the socket after finish is emitted, but that seems like an edge case.

I think the router should be updated to not emit finish (end?) until finish is emitted by the Response.

Someone should probably dive into live examples of this flow a bit more first. I can be that someone, but my timeline would be a bit open.

[szmarczak]

Given this example:

const request = http.request('http://example.com');

request.once('finish', () => {
	console.log('finish');
});

request.once('end', () => {
	console.log('end');
});

request.abort();
request.end();

You will see that finish (nor end) is not emitted. That's the correct behavior.

I think the router should be updated to not emit finish (end?) until finish is emitted by the Response.

IncomingMessage has no finish event...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

[mastermatt]

I forgot about this over the holidays, but good ol' Stale Bot did its job.

First off it seems, something in Got was changed between 10.0.2 and 10.0.3, because running the "Code to reproduce" from the original Got ticket now rejects the promise (I think this commit).
That being said, I still think there is a bug in how Nock emits events on its ClientRequest.

It should not emit an end event at all. My gut says this was a copy/pasta bug from the event on the response.

IncomingMessage has no finish event...

@szmarczak agreed! But I was referring to the finish event emitted by the Nock router to emulate the event on the OutgoingMessage.
We could postpone this event until just before playbackInterceptor is called. That way it's not called if no interceptor is matched and the request ends up not being mocked.

But I'm having a hard time justifying that it's not ok for a ClientRequest to have a finish event followed by an error event.
If anything I think we should postpone the finish for the case where the request ends up under the allowUnmocked flow and a "real" request is written to. Currently for that flow, finish will be emitted twice.

Lastly, when I was playing around with different versions of these code reproductions, both with and without Nock for comparisons, I noticed that Nock is emitting errors when OutgoingMessage methods are called after request.abort(). There are tests that verify the behavior, but no comments as to why we deviated from native Node. @paulmelnikow do you happen to know?

I have some changes locally that I'll clean up and PR, but I want to sleep on it first.

[github-actions]

🎉 This issue has been resolved in version 12.0.3 🎉

The release is available on:

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

Your semantic-release bot 📦🚀