mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
523 stars 118 forks source link

test: add `setTimeout` tests #558

Closed kettanaito closed 2 months ago

kettanaito commented 2 months ago

Timeout was supported as-is, I've only added automated tests.

[!IMPORTANT] http.request(u, { timeout: n }) and http.request(u).setTimeout(n) are NOT the same! The first acts on the http.Agent level, adding timeout to every net.Socket once it's created. The second, however, ignores the agent and sets the timeout on the socket once it emits the "connect" event. The first takes the connection time into account, the second doesn't.

I've opened a pull request to the Node.js docs (https://github.com/nodejs/node/pull/52576) to help emphasize this difference.

In the context of Interceptors, this difference is important because we can reproduce http.request(u, { timeout: n }) by making the request listener take more time than the specified timeout, but the http.request(u).setTimeout(n) must receive either a mocked or bypassed response headers before it's even added (socket connect emitted).

The latter is, technically, our limitation because we cannot emit connect on the socket until we know whether the request is mocked or bypassed. In practice, that doesn't matter because you are either mocking a response or not. We treat the mocked response headers as the initiator of the socket connection.

Also adds a this.destroyed check in passthrough() and respondWith() so they'd have no effect if the socket has been destroyed (e.g. aborted or timed out).

kettanaito commented 2 months ago

@mikicho, I think you should know about the difference I outlined above. See if Nock understands those two different timeouts and treats them the same. It's rather tricky and undocumented, and I wouldn't be surprised if it didn't.

kettanaito commented 2 months ago

I also suspect that AbortSignal.timeout(n) will act similarly to setting a timeout. But I expect it to result in the "abort" event on the request.

mikicho commented 2 months ago

See if Nock understands those two different timeouts...

Nock is aware of these two, and it actually uses this value for throwing a timeout error immediately if the timeout is bigger than what is defined in the interceptor. This is a nifty feature that MSW (or mswjs/intrceptors) should consider.

P.S: when I tried to implement this for the fetch API, it turned out that the delay connection feature doesn't make much sense because Fetch doesn't provide this level of granularity.

kettanaito commented 2 months ago

This is a nifty feature that MSW (or mswjs/intrceptors) should consider.

Interceptors do not decide that for you so they shouldn't do anything on top of what Node.js does. Node.js doesn't do anything to the request, not even destroys it. It only emits the timeout event for you do handle that timeout in whichever fashion you want. I think it should stay that way.

delay connection feature doesn't make much sense because Fetch doesn't provide this level of granularity.

Yep, I remember that!

mikicho commented 2 months ago

Interceptors do not decide that for you so they shouldn't do anything on top of what Node.js does

Nock does not change Node.js behavior here:

  1. with "auto timeout,": timeout event emits immediately.
  2. Without: Nock waits X seconds and emits a timeout event.

Users still can use fake timers instead, but Nock give this OOTB which is nice.