Feat: Added response.destroy method

[b4dnewz]

It adds the destroy method to the response to manually terminate a connection and optionally raise an error captured from listeners bound to request error event.

As discussed in #1669

---

I'm still not sure if this is enough, I took a look at http_incoming from node http core:

IncomingMessage.prototype.destroy = function destroy(error) {
  if (this.socket)
    this.socket.destroy(error);
};

And from http_client also from node http core:

if (ret instanceof Error) {
    prepareError(ret, parser, d);
    debug('parse error', ret);
    freeParser(parser, req, socket);
    socket.destroy();
    req.socket._hadError = true;
    req.emit('error', ret);
} ...

And based on the description provided in the documentation:

https://nodejs.org/api/http.html#http_class_http_incomingmessage

It will raise the error event only if called with an actual Error instance.

But since I'm not familiar with node http core lib I'm not sure if this will terminate the connection properly or is required to manually perform other tasks on the request object.

[paulmelnikow]

Thanks for digging into this! 💯

A couple follow-up thoughts:

… I took a look at http_incoming from node http core:

IncomingMessage.prototype.destroy = function destroy(error) {
  if (this.socket)
    this.socket.destroy(error);
};

This is a bit curious given the description in the documentation. Does socket.destroy() somehow trigger the behavior of emitting the error event on the request?

And from http_client also from node http core:

if (ret instanceof Error) {
    prepareError(ret, parser, d);
    debug('parse error', ret);
    freeParser(parser, req, socket);
    socket.destroy();
    req.socket._hadError = true;
    req.emit('error', ret);
} ...

Regarding this piece of code – I'm not sure – is it relevant to req.destroy()?

[b4dnewz]

Regarding this piece of code – I'm not sure – is it relevant to req.destroy()?

I'm not sure either, but might be the part that raise the error event on the request, I could not find any other relevant look-a-like code in http_client file.

Does socket.destroy() somehow trigger the behavior of emitting the error event on the request?

I really don't know, from what I see in the http_client.js core file look it won't.. but I didn't test it without, I mostly looked at the implementation of abort on request_overrider.js file from nock and used the emitError method.

Also maybe we should check for the socket existence before calling the destroy method to avoid calling on an already destroyed socket as said in the description (I cutted out) from the first file:

It's possible that the socket will be destroyed, and removed from
any messages, before ever calling this. In that case, just skip
it, since something else is destroying this connection anyway.

[mastermatt]

I don't think overriding the destroy method on the response is the right approach. The response is an actual instance of IncomingMessage, which means it already has the correct method available.

The bit that's lacking is our Socket surrogate class. Comparing the current implementation vs the docs, shows that it doesn't handle being passed an error correctly.

If exception is specified, an 'error' event will be emitted and any listeners for that event will receive exception as an argument.

nock/lib/socket.js

Lines 57 to 60 in 4d0fbbf

	destroy() {
	this.destroyed = true
	this.readable = this.writable = false
	}

If you're curious what Node does normally, the this.socket.destroy(error); line in IncomingMessage.destroy kicks off this function.

Updating the destroy method in our Socket class to something like the following should get us close enough to the actual logic.

  destroy(err) {
    this.destroyed = true
    this.readable = this.writable = false
    if (err) {
      this.emit('error', err)
    }
    return this
  }

The one other thing that would need to change would be to update this line so that error events also propagate through the request.

nock/lib/request_overrider.js

Line 114 in 4d0fbbf

propagate(['timeout'], req.socket, req)

This is where the propagation happens in Node. It probably doesn't make sense to add the other missing events at this time. Wait to see if there is an actual need.

With those two changes, the tests @b4dnewz wrote for this PR passed for me locally.

[b4dnewz]

I don't think overriding the destroy method on the response is the right approach.

I also initially had some doubt about it, your observations are good and a cleaner way to do it, I will update the code with your suggestions.

[paulmelnikow]

Is return this needed?

[mastermatt]

That's the documented behavior.

[paulmelnikow]

Ah, I see it now. Thanks.

[nockbot]

🎉 This PR is included in version 11.2.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[nockbot]

🎉 This PR is included in version 11.2.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀