Should we run interceptor handler for destroyed(/aborted) requests?

[mikicho]

In Nock, we have this test:

  it.only('Emits the expected event sequence when `end` is called on an aborted request', done => {
    const scope = nock('http://example.test').get('/').reply()

    const req = http.request('http://example.test')
    const emitSpy = sinon.spy(req, 'emit')
    req.abort()
    req.end()

    setTimeout(() => {
      expect(emitSpy).to.have.been.calledTwice
      expect(emitSpy.firstCall).to.have.been.calledWith('close')
      expect(emitSpy.secondCall).to.have.been.calledWith('abort')
      expect(scope.isDone()).to.be.false()
      done()
    }, 10)
  })

This test fails on this line:

expect(scope.isDone()).to.be.false()

because we handle the end event as usual even for aborted requests.

[mikicho]

Also, we have this test:

  it.only('Emits the expected event sequence when aborted immediately after `end`', done => {
    const scope = nock('http://example.test').get('/').reply()

    const req = http.request('http://example.test')
    const emitSpy = sinon.spy(req, 'emit')
    req.end()
    req.abort()

    setTimeout(() => {
      expect(emitSpy).to.have.been.calledTwice
      expect(emitSpy.firstCall).to.have.been.calledWith('close')
      expect(emitSpy.secondCall).to.have.been.calledWith('abort')
      expect(scope.isDone()).to.be.false()
      done()
    }, 10)
  })

Which first ends the request and then aborts it.
I don't know the desired behavior here (from mocking POV)

[kettanaito]

@mikicho, this is what would happen if you end the request and then abort it:

1. The request event will get emitted.
2. Nothing you do in the request listener will matter.
3. The library will catch the abort event and forward it to the request instance as it would normally.

I believe this makes sense intention wise—aborted requests cannot have responses.

We've recently improved this behavior in #394, and I believe ClientRequest should respect that also (although I don't recall us handling options.signal for that matter).

What does Nock expect in this case?

[mikicho]

@kettanaito

this is what would happen if you end the request and then abort it.

Nock tests two scenarios (attached above)

1. Frist abort. then end
2. First end, then abort

What does Nock expect in this case?

In both cases, Nock (or node RequestClient) emits an abort event, and the interceptor logic shouldn't run (`scope.isDone() should be false)

this is what would happen if you end the request and then abort it

I believe the issue is that we execute the interceptor logic normally when req.end is triggered for an aborted/destroyed request.

[kettanaito]

Since those two scenarios are a bit different, we will handle them differently.

.abort() + .end()

I expect .abort() resulting in this.destroyed() to be true. We can check that property in the .end() method, and if it's the case, perhaps do super.end()...? Not sure what's the right handling here. Also depends what Node itself would do in this scenario. I'd expect it to throw an error but I haven't tested that to confirm.

.end() + .abort()

This is more tricky since .end() kicks off the request listener lookup while .abort() is synchronous but can happen at any time during that lookup, e.g. when it's already in progress and has called some listeners.

There's this.signal on request we can listen to if it's always set by Node. It's an abort controller signal and it can help us detect when request has been aborted.

Since abort after end still indicates a finished request (we are effectively aborting receiving the response, the request has been sent OK), I think the easiest approach is to:

1. Allow the request listeners to run, gather their result.
2. Right after the until() promise resolves, check this.destroyed (or the abort signal, any way we choose to go), and if the request has been aborted, likely do super.end() to make Node throw an error it would normally (again, writing this on a premise Node throws in that case).

[mikicho]

Nock has an extensive testing suite around the abort scenario.
I suggest we add Nock's tests "as-is" (as possible) and make them pass. It would help us to find more concrete examples/problems.
WDYT?

I'd expect it to throw an error, but I haven't tested that to confirm.

From what I see, Node doesn't throw, instead, it emits abort and close events, in both cases.
According to Nock tests, it emits in a more complex scenario

Allow the request listeners to run, gather their result.

From Nock POV. Yes, except it shouldn't arrive at the markConsumed step which is almost at the end of the interceptor logic. I think we would be OK.