Closed mikicho closed 2 months ago
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)
@mikicho, this is what would happen if you end the request and then abort it:
request
event will get emitted. 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?
@kettanaito
this is what would happen if you end the request and then abort it.
Nock tests two scenarios (attached above)
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.
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:
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). 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.
I think we can close this one.
In Nock, we have this test:
This test fails on this line:
because we handle the
end
event as usual even for aborted requests.