switch to http-proxy-3

[minrk]

• Context: node-http-proxy has gone unmaintained for a long time, leading to maintenance issues like Socket leak  #434 and others. http-proxy-3 is a fork which aims to be a healthy, maintained, modern version.
• Breaking change for some users: http-proxy-3 requires node 18, which means only ubuntu 24.04 LTS / Debian 12 (stable) are the minimum versions who can get supported node from apt, where is was the ancient node 10, previously. Users already on node >=18 (e.g. in containers, z2jh, etc.) should see no changes.

Once merged, we can update a few dev dependencies that have also been held back because of our minimum version constraint.

requires node 20, a major change since no LTS has it.

cc @williamstein

this is a major change in minimal nodejs, since v20 is newer than any version available in a stable distro (ubuntu 24.04 LTS and debian stable both have v18), so it means ~everyone needs to get node separately or use containers/conda/etc. Pro: almost everyone already does that, but it's not quite everyone.

[minrk]

Looks like one test is failing, the one using autoRewrite: true, which is actually an http-proxy feature:

    TypeError: Invalid URL
        at new URL (node:internal/url:819:25)
        at setRedirectHostRewrite (/Users/minrk/dev/jpy/configurable-http-proxy/node_modules/http-proxy-3/dist/lib/http-proxy/passes/web-outgoing.js:79:24)
        at ClientRequest.<anonymous> (/Users/minrk/dev/jpy/configurable-http-proxy/node_modules/http-proxy-3/dist/lib/http-proxy/passes/web-incoming.js:166:17)
        at ClientRequest.emit (node:events:507:28)
        at HTTPParser.parserOnIncomingClient [as onIncoming] (node:_http_client:716:27)
        at HTTPParser.parserOnHeadersComplete (node:_http_common:117:17)
        at Socket.socketOnData (node:_http_client:558:22)
        at Socket.emit (node:events:507:28)
        at addChunk (node:internal/streams/readable:559:12)
        at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)

haven't had a chance to look into if it's something we're doing or something in http-proxy-3. Seems likely to be related to the switch to URL from string parsing, but we have no code related to rewriting.

[williamstein]

@minrk I'll spend some time right now to see whether or not it is reasonable to officially support node 18 with http-proxy-3. I hadn't because officially node v 18 seems to be end of life'd according to https://nodejs.org/en/about/previous-releases.

[williamstein]

@minrk I'll spend some time right now to see whether or not it is reasonable to officially support node 18 with http-proxy-3. I hadn't because officially node v 18 seems to be end of life'd according to https://nodejs.org/en/about/previous-releases.

The main issue with supporting node 18 is that the fetch built into node causes a problem for many of the unit tests, leaving an open handle. Fortunately, we already have node-fetch as a dev dependency, and switching to that for the unit tests works fine with node 18, so I'll do that. So looking good.

[minrk]

Yeah, not a problem! I think it's a fair choice for anyone to make - we just hadn't had a reason to bump in a long time, so it's a big jump. If the version in Debian stable/ubuntu LTS is easy enough to support, I think that makes things a bit easier to install for some users, but no worries if there's a good reason to require 20.

[williamstein]

If the version in Debian stable/ubuntu LTS is easy enough to support, I think that makes things a bit easier to install for some users, but no worries if there's a good reason to require 20.

It turns out there are no good reasons to require 20. I just pushed a new patch version (1.20.1) of http-proxy-3 that supports node versions 18, 20, 22, and 24, meaning that package.json reflects that and CI runs on all four node versions. There are 3 or 4 unit tests that are disabled with node 18, but otherwise no library changes were required at all (it's just testing code). The tests pass but hang with "A worker process has failed to exit gracefully and has been force exited." only on node 18.

[williamstein]

like one test is failing, the one using autoRewrite: true, which is actually an http-proxy feature:

I looked and as you suggested, the code in http-proxy-3 using new URL didn't have a second default argument, so it wouldn't support relative url's. I put in "http://dummoyorg" as a second argument so pathname, etc., can be extracted in such cases as before, giving the same behavior as url.parse. This is in version 1.20.2 of http-proxy-3.

[minrk]

Still seem to be getting the same error with 1.20.2. I’ll try to isolate it into an http-proxy test to make sure it’s not something on our side.

[williamstein]

Still seem to be getting the same error with 1.20.2. I’ll try to isolate it into an http-proxy test to make sure it’s not something on our side.

If you have trouble tracking it down let me know. I could change this exception

TypeError: Invalid URL
        at new URL (node:internal/url:819:25)

to also include the invalid URL, since we are wondering what URL jupyterhub is trying to rewrite...

[minrk]

@williamstein I figured out that it's an incompatibility with old url.parse. Apparently you can't do new URL(Url). We pass a parsed url.parse as target which used to be supported, but this fails, because:

const urllib = require("url");
new URL(urllib.parse("http://127.0.0.1")); // fails with TypeError

Accessing Url.href would work.

This should be easy to fix on our end by switching to URL from Url. It's technically a breaking change, so you might want to note it, but since the old url.parse API has been deprecated since node 11, I'm not sure you need to fix it.

[williamstein]

It seems harmless to support urllib.parse, so I've done so:

sagemathinc/http-proxy-3@63d3a62

I also put all calls to "new URL" in a single function, which makes it easier to follow the code.

This is now version 1.20.3.

[minrk]

Thanks, @williamstein! I made the URL updates here, too.

@consideRatio @manics I think this is ready to go.

After this, I can land a few other dependency bumps that have been blocked on the node update, and then maybe make a prerelease to see how this goes?

[consideRatio]

I'll find time in a day or two, I'm currently in a push to get another thing finalized.

[consideRatio]

I inspected the changes line by line and considered some nearby code context, it looks fine to me!

Communication wishes:

• add reference to context or inline some context on why we switch in the pr description
• clarify if/how this is a breaking chance in the pr description

I see it as a breaking change since we bump the node versions lower bound. I figure it doesnt make z2jh bumping to this require a breaking change though.

Happy to see a self-merge now or a wait until for example friday for Simon to chime in with a notable change like this. I havent followed the discussion, maybe Simon is already in agreement!

[minrk]

@consideRatio thanks! I added some detail. I think this is a breaking change for that reason, though not for most users, and definitely not for z2jh.