Repository navigation
Mockttp is not working with Node.js 19 / Wrong connection header handling #107
Description
Activity
After looking into failing tests, they are really just about default keepalive header. The issue I have is that mockttp doesn't receive all calls it is supposed. So, I need to really find replication for it.
Hi @freaz, yes, I think you're right - most errors there are just because Mockttp's tests assume that Node.js doesn't use keep-alive by default. That's just an issue with the tests, not the library in general, but yes it would be good to fix that.
If you have a minute, a PR would be very welcome! We already have various cases in the tests for other differences between node versions, and I think we can probably do the same here (i.e. add a new constant, and change the assert in the tests to check
process.versionand look for different headers depending on the node version).I haven't looked into Node 19 much for Mockttp yet. I suspect there'll be some other differences in here too, but as far as I'm aware there's nothing that would break real functionality, just low-level assumptions within in our own test suite.
Please excuse delay, we all are busy with a bunch of stuff. I played a bit more with mockttp and
keepalivebehaviour.And I am convinced I found isolated replication for issue I encounter.
It is for sure related to
keepaliveas default change in Node.js 19, but the example below fails with any Node version ifkeepaliveis set to true on agent.https://github.com/freaz/mockttp_keepalive_bug/blob/main/index.js
Another observation is that the issue occurs when used
thenJsonbut not withthenReply.I will continue finding the root cause and fix if possible. But I want to share it, if someone with knowledge of mockttp internals have an idea what it could be and could save me some time.
I believe the root cause is dropping default headers, it drops
connectionheader and thus, client don't know how to behave.https://github.com/httptoolkit/mockttp/blob/main/src/util/request-utils.ts#L102-L112
And I think implementation isn't correct, because headers might be set also internally. For example when setting content-type and content-length for json responses.
Commit adding default headers drop: 738ac2e
- changed the title
[-]Mockttp is not working with Node.js 19[/-][+]Mockttp is not working with Node.js 19 / Wrong connection header handling[/+]on Jan 12, 2023 I did some digging, and I'm fairly sure this is a Node bug. I've filed an issue here: nodejs/node#46321. They seem to broadly agree, and I've opened a PR that should fix this in future.
The underlying problem: although we're removing the Connection header (because
thenJsonautomatically defines some response headers, and doesn't specify a Connection header there) that shouldn't matter, because for HTTP/1.1+ keep-alive is the default anyway. The reason the client is failing is because it's expecting the connection to be kept alive, but Node's HTTP implementation incorrectly automatically kills connections after responses if you remove the Connection header.What should actually happen is that Node should try to keep the connection alive, even if you remove the header, unless you explicitly send a "Connection: close" header. If it did that, then Mockttp would work correctly here. The PR to fix that is here: nodejs/node#46331
Fortunately, it looks like we may well be able to get this fixed in Node, which would resolve this long term. In the short term, I'm going to quickly patch this in Mockttp too, by adding an explicit 'Connection: keep-alive' header to the default headers set for
thenJsonresponses, to ensure Node behaves correctly in that case. In future we might be able to limit that to only affected Node versions, but for now we'll just set it everywhere.This has now been released in Mockttp v3.6.3:
thenJsonwill now automatically set the keep-alive header, and so Node will correctly keep the connection alive as expected.In addition, the Node team have merged my PR there too. Once that's shipped (in the next major release, i.e. Node v20 in April) then this should work in new Node, even with older Mockttp releases. Once that's live I'll retest and add a check to skip that header again in versions of Node where it's not required.
Thanks for reporting this! Great to be able to fix it here and fix node long-term for everybody else 👍
I ran into issue with mockttp after updating to Node.js 19.
Unfortunately, I couldn't figure out what is the root cause and prepare replication. I assume it is related to updated llhttp or keepalive by default.
Here is fork with Node.js 19 in testing matrix which is failing:
https://github.com/freaz/mockttp/actions/runs/3395295030/jobs/5644990838