Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add setTimeout tests #558

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Apr 18, 2024

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 (nodejs/node#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 kettanaito changed the base branch from main to feat/yet-another-socket-interceptor April 18, 2024 10:29
@kettanaito
Copy link
Member Author

@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 kettanaito changed the title test: add socket timeout tests test: add setTimeout tests Apr 18, 2024
@kettanaito
Copy link
Member Author

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.

@kettanaito kettanaito merged commit e52851c into feat/yet-another-socket-interceptor Apr 18, 2024
2 checks passed
@kettanaito kettanaito deleted the fix/socket-timeout branch April 18, 2024 13:01
@mikicho
Copy link
Contributor

mikicho commented Apr 18, 2024

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
Copy link
Member Author

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
Copy link
Contributor

mikicho commented Apr 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants