Fix: Correctly pass http version to the ServerResponse in node#756
Fix: Correctly pass http version to the ServerResponse in node#756RobinVdBroeck wants to merge 3 commits intomswjs:mainfrom
Conversation
RobinVdBroeck
changed the title
| // By default, nodejs sets keepalive to false if the HttpVersion is not at least | ||
| // 1.1: https://github.com/nodejs/node/blob/70f6b58ac655234435a99d72b857dd7b316d34bf/lib/_http_server.js#L211C1-L211C34 | ||
| // The version should normally already have been set by the onRequestStart method | ||
| if(this.httpVersionMajor != null && this.httpVersionMinor != null) { |
There was a problem hiding this comment.
Hm, this is interesting! From what I gathered, this IncomingMessage was purely internal to help us handle the mock response and shouldn't influence the user-facing behaviors in any way. So you say it does?
This might be an indicator that we should consider migrating from this.
All ServerResponse does here is help us parse out the response. User-facing ClientRequest constructs its own instances, including IncomingMessage, and ours never leaks or affects the user-facing data in any way. At least, that is the intention.
| ____, | ||
| shouldKeepAlive | ||
| ) => { | ||
| this.httpVersionMajor = versionMajor |
There was a problem hiding this comment.
A single socket instance might be reused across different requests (decided by the agent). If we decide to go with this approach, we need to reset these properties at some point (e.g. at new request start).
|
Hi, @RobinVdBroeck. Thank you for looking into this, and an even bigger thank you for adding those tests! I would have to come back to this next year and see if there is any other way we can address this issue. |
2 participants
This PR updates the MockHttpSocket to correctly set the http version passed to ServerResponse. This fixes a bug where mocked responses with the Connection: keep-alive header never completed before. For that, I've added some regression tests.
I found it kinda hard to wrap my head around the architecture of mswjs/interceptors, so please let me know if I did something stupid.
This fixes a bug in nock: nock/nock#2918