fix(ClientRequest): prevent requests issued by "follow-redirects" from hanging infinitely #260
Conversation
95th
changed the title
| expect(request.body).toBe(JSON.stringify({ todo: 'Buy the milk' })) | ||
|
|
||
| // Response (original). | ||
| expect(await text()).toBe('hello from the server') |
There was a problem hiding this comment.
I've added assertion for the received response so we're sure that follow-redirects was able to redirect the request to the server.
|
|
||
| req.end(payload) | ||
|
|
||
| const { text } = await waitForClientRequest(req as any) |
There was a problem hiding this comment.
Added this await line so whenever this request rejects, the test rejects also. Previously, the test would pass if requesting a non-existing resource/hostname.
| const { address } = server.https | ||
| const payload = JSON.stringify({ todo: 'Buy the milk' }) | ||
|
|
||
| const req = https.request({ |
There was a problem hiding this comment.
@95th, do you know what's the intention behind using follow-redirect usually is?
I somehow suspected that this package is meant for HTTP -> HTTPS automatic redirects but I'm probably wrong. I wonder if we're reflecting how people use follow-redirects in this test.
There was a problem hiding this comment.
Okay, my initial assumption about follow-redirects was wrong.
Having read their readme, I can see that the purpose it exists is to automatically follow redirects that occur during requests. For example:
GET https://bit.ly/abc-123
- (redirects to https://api.github.com/user
RESPONSE:
- From GitHub, not bit.ly
I will restructure our test so reflect this usage scenario.
There was a problem hiding this comment.
I understand that it may feel unnecessary to assert that follow-redirects actually perform a redirect request but we should still do so. In this assertion, we're making sure that Interceptors don't interfere with that behavior, and that the redirects still happen.
| if (!chunk || chunk.length === 0) { | ||
| this.log('written chunk is empty, skipping callback...') | ||
| } else { | ||
| callback?.() |
There was a problem hiding this comment.
Note for posterity: so instead of scoping the custom write callback in the this.chunks and replaying those in .end(), we're calling write callbacks immediately now. Since we do, now intercepted requests won't get stuck due to follow-redirects calling req.end() as a part of req.write() callback (previously, pending indefinitely).
kettanaito
changed the title
kettanaito
changed the title
There was a problem hiding this comment.
Thank you so much for tackling this, @95th! 🎉 Welcome to contributors.
If you find anything else in the MSW ecosystem that interests you, or you feel you could improve, don't hesitate to ping me in the respective issues, or open a pull request.
|
@kettanaito Thanks for the fix up and merging. Any ETA on when it will become published and propagate to msw ? |
|
Everything in the MSW ecosystem is released nightly. So, tonight.
You can always resolve a dependency at a specific PR state. Use Yarn resolutions or NPM overrides to do that. |
Released: v0.16.5 🎉This has been released in v0.16.5! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Changes
.write()callback immediately vs doing so in the.end()ofNodeClientRequest.