POC: Request through undici #34
Conversation
|
It seems to me that the best option is "Create a new API for http2 requests (which then could also be used for http1/http1.1 used)". This keeps things backward compatible -- which is a hard requirement for this project -- but still provides a path for http2 support. |
|
Probably makes sense. I started to go in this direction by adding "clientOptions" and "requestOptions", which are undici specific. If either of those are set, the new code path will trigger. The next step will be assessing the compatibility of the existing options with this new code path. Then I will be able to figure out when and how to warn for incompatible options . For example, setting dispatchOptions and a custom http agent will never be compatible, while |
|
Staring at this stuff for a few more hours I realized that most of the options stuff happens outside of the "stream" function anyway. With the current modifications almost all tests pass, the exceptions being only
So very promising so far. My next steps would be documentation on the new available options and their implications. And to provide some callback-based alternatives to |
|
I have converted this into a normal pull request, adding a secondary stream path using undici. This code path is triggered by setting the undici option to a truthy value and provides the following options. Its main advantages are full http2 support and better performance.
I updated the documentation to describe these new features - and marked undici code path as experimental. The undici code path can also be triggered by setting a ´FORCE_UNDICI_PATH´ environmental variable - this is mainly used to run the tests also for the undici code path - only 4 tests are skipped then. The main point I am hessitant about is the specific |
|
I haven't studied the PR enough yet, but I personally like your approach of being explicit about using undici, and also that the http2 functionality is behind that flag (or env variable). I really like the approach you've taken. I have a question about install size, which just occurred to me. Right now the https://www.npmjs.com/package/http-proxy-3 package is 87 KB, and I think there are some users that really care about the install size. With undici, I guess the install size will increase to at least 2MB, since the install size of undici is 1.47 MB. Have you thought about how to address this?
|
|
Thanks for your encouraging words! Regarding size, that is a tough one and not something I was aware of. But during the night I thought of something else that might also help here. I am still not sure if going undici really is the best way forward. Alternatively I could switch the code to use But it could also be made to support passing a custom import {fetch, Agent, setGlobalDispatcher} from "undici"
function myFetch (url, opts) {
opts ||= {};
opts.dispatcher = new Agent({allowH2: true});
return fetch(url, opts);
}
const proxy = createProxyServer({fetch: myFetch})
// Alternatively (but this affects all of fetch)
setGlobalDispatcher(new Agent({allowH2: true})While a bit cumbersome, this should be more future proof and http2 support in fetch could also be the default in future node.js versions. More importantly this will be independent of the javascript runtime, so for example deno's fetch already supports http2 requests. I just tried a bit testing with bun and bun and undici do not seem to be completely compatible yet. |
|
Yes, I really like the idea of passing something imported from undici (or whatever) in, given the role of http-proxy-3 as a long-term backward compat not too big library. |
|
Can't wait for this to get merged! Amazing job |
|
@Corvince What's the status? Also, I wonder if a different approach to avoid blowing up the package size is making undici a "peer dependency"? @manucorporat It's sitting just because I'm not sure of the status. There was a working version that made http-proxy-3 much larger (due to always installing undici), but then it got refactored to pass undici in... but that maybe doesn't pass the unit tests. |
|
I had some blockers before, but now this is alive and kicking again! Basic functionality is working again now without undici and just using normal fetch. Need to fix some things still and also update the documentation heavily, which is still using undici. The test errors are somewhat inflated because it is using fetch-api + websocket path, which do not play well together yet. I'll provide some more details tomorrow |
|
So, this mostly works as intended. Some tests are still failing which need fixing. They belong to 2 categories
const ports = { source: gen.port, proxy: gen.port };
const source = http
.createServer((req, res) => {
expect(req.method).toEqual("GET");
expect(req.headers.host?.split(":")[1]).toEqual(`${ports.proxy}`); // <== This is failing
res.writeHead(200, { "Content-Type": "text/plain" });
res.end("Hello from " + ports.source);
})
.listen(ports.source);Specifically they test if the "host" header includes the proxy port. |
Corvince
force-pushed
the
http2-incoming
branch
from
d27564b to
2bd026b
Compare
|
We have some big project handling a lot of traffic under http-proxy-3, I will be happy to test some percentage of the traffic under undici, if you guys can ship some sort of pre-release. happy to report and debug any issues that arises. |
|
To push things forward, I could deactivate the additional tests. The normal functionality works now, especially handling and proxying http2 requests. The additional tests are run with a forced fetch: true setting. Again, some tests fail because the host header is handled differently, which can't be changed. And some websocket tests fail, but we should mark the fetch option as experimental for now anyways. And finally some tests that rely on custom error handling are failing. This needs more more work, but could potentially be handled in a follow up pr. This would mean I could just update the documentation and deactivate the fetch forced tests and then @williamstein you could do a new release for @manucorporat to test things out. Otherwise it's gonna take some more time, because at least this week I won't be able to work on this |
|
That sounds like a good plant to me. The key thing is that I just don't want to break http-proxy-3 for any existing users of the current longterm stable api. Adding new not-100% functionality that is only visible with customer configuration is OK, especially if it leads to extra testing. |
|
Okay, then I will do so tonight or tomorrow. Currently there should be no breaking change to the existing code paths so this should be fine |
Corvince
force-pushed
the
http2-incoming
branch
from
b90a6b2 to
ddf7f05
Compare
…to http2-incoming
|
I have made the changes, but unfortunately in trying to remove the whitespace changes I introduced even more. The downside of auto-formatting on save in a non-owned repo ;) |
|
This already looks really good. I have skimmed over everything, but want to take a more careful look tomorrow. If anybody else reading this has any comments or concerns, please let us know. I love that this doesn't directly add new deps, and the new docs with the table are clear. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds experimental HTTP/2 support to http-proxy-3 via the fetch API with callback-based request/response lifecycle hooks. The implementation introduces a dual-path architecture where proxying can use either the traditional Node.js native HTTP modules or the new fetch-based approach for HTTP/2 support.
Key Changes:
- New fetch code path with
stream2function supporting HTTP/2 via undici dispatcher - Callback-based lifecycle hooks (
onBeforeRequest,onAfterResponse) for fetch path - Conditional test skipping to handle path-specific test cases
- Comprehensive documentation for HTTP/2 configuration and usage
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added test scripts for dual-path testing (native and fetch) |
| lib/index.ts | Changed http import from namespace to type-only import |
| lib/http-proxy/index.ts | Added FetchOptions interface and fetch configuration support with extensive type refactoring |
| lib/http-proxy/passes/web-incoming.ts | Implemented stream2 function for fetch-based proxying with HTTP/2 support |
| lib/http-proxy/passes/web-outgoing.ts | Added HTTP/2 header filtering logic (connection, keep-alive) |
| lib/http-proxy/common.ts | Added HTTP2_HEADER_BLACKLIST and url property to outgoing options |
| lib/test/middleware/body-decoder-middleware.test.ts | Wrapped in skipIf to exclude from fetch code path |
| lib/test/lib/https-proxy.test.ts | Commented out host header port assertions |
| lib/test/lib/http-proxy.test.ts | Added skipIf conditions for native-only tests, reformatted code |
| lib/test/lib/http-proxy-passes-web-incoming.test.ts | Added skipIf for proxyReq event tests, updated timeout error expectations |
| lib/test/http/proxy-http2-to-http2.test.ts | New test suite for HTTP/2 to HTTP/2 proxying |
| lib/test/http/proxy-callbacks.test.ts | New test suite for fetch callback functions |
| lib/test/http/error-handling.test.ts | Import formatting change |
| README.md | Extensive documentation for HTTP/2 support, fetch configuration, and code path compatibility |
| .github/workflows/test.yml | Updated to run native HTTP tests only |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect((err as NodeJS.ErrnoException).code).toBeOneOf([ | ||
| "ECONNRESET", | ||
| "UND_ERR_HEADERS_TIMEOUT", | ||
| ]); |
There was a problem hiding this comment.
The updated test now expects either "ECONNRESET" or "UND_ERR_HEADERS_TIMEOUT" error codes. The UND_ERR_HEADERS_TIMEOUT error code is specific to undici (the fetch implementation), which suggests this test may behave differently in the fetch code path. However, the test is not conditionally skipped for either path. Consider whether this test should:
- Be split into separate tests for native and fetch paths with appropriate error expectations, or
- Only expect the error code that's common to both paths
The current approach of accepting either error code may mask issues where the wrong code path is being tested.
There was a problem hiding this comment.
This is very true and as I said earlier, (custom) error handling for the new code path is definitely something that needs to be looked at again in some follow up work! For now it doesn't hurt to have it, although it might not be comprehensive enough (or probably the fetch path could unify the error code)
|
I ran an AI review and it did point out some good questions, e.g., if (_req.httpVersionMajor > 1 && (key === "connection") || key === "keep-alive") {looks suspicious. Can you look over those comments? Of course, I also am manually reading the PR. |
|
Good points by the AI, I resolved/ accepted the suggestions in all of them except one, which I would prefer to be addressed in a follow up. |
|
We could delete this line from the README: |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
README.md:809
- Code block syntax error: lines 804-809 are outside the previous code block (which ends at line 802) but are formatted as JavaScript code. These lines should either be:
- Removed if they're leftover from the previous example, or
- Wrapped in their own code fence (
js ...) if they're a separate example.
Currently this breaks the markdown formatting.
// Listen for the `close` event on `proxy`.
proxy.on("close", (res, socket, head) => {
// view disconnected websocket connections
console.log("Client disconnected");
});
</details>
---
💡 <a href="/sagemathinc/http-proxy-3/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| options: NormalizedServerOptions, | ||
| ) => void | Promise<void>; | ||
| /** Custom fetch implementation */ | ||
| customFetch?: typeof fetch; |
There was a problem hiding this comment.
The customFetch option is defined here but never used in the implementation (stream2 function in web-incoming.ts always uses the global fetch). Either implement support for using customFetch when provided, or remove this option from the interface to avoid confusion.
| customFetch?: typeof fetch; |
There was a problem hiding this comment.
It has a good point, since:
wstein@black:~/build/http-proxy-3$ rg customFetch
README.md
544:function customFetch(url, opts) {
554: customFetch,
671: - `customFetch`: Custom fetch implementation to use instead of the global fetch
lib/http-proxy/index.ts
126: customFetch?: typeof fetch;
wstein@black:~/build/http-proxy-3$ There is no mention of customFetch in the actual code.
| const requestOptions: RequestInit = { | ||
| method: outgoingOptions.method, | ||
| }; | ||
|
|
||
| if (fetchOptions.dispatcher) { | ||
| requestOptions.dispatcher = fetchOptions.dispatcher; | ||
| } |
There was a problem hiding this comment.
The requestOptions object is being constructed without headers for the forward path, but headers are set for the target path (line 289). This inconsistency means that options like auth and custom headers from options.headers won't be applied to forward requests. Consider extracting the header construction into a shared function or ensuring both paths handle headers consistently.
| requestOptions.body = options.buffer as Stream.Readable; | ||
| } else if (req.method !== "GET" && req.method !== "HEAD") { | ||
| requestOptions.body = req; | ||
| requestOptions.duplex = "half"; |
There was a problem hiding this comment.
The duplex: "half" option is set unconditionally for non-GET/HEAD requests. This is required for streaming request bodies in fetch, but the TypeScript type definition for RequestInit might not include this property. Consider adding a type assertion or ensuring the types are properly extended to avoid potential type errors:
requestOptions.duplex = "half" as any;| requestOptions.duplex = "half"; | |
| requestOptions.duplex = "half" as any; |
| requestOptions.body = options.buffer as Stream.Readable; | ||
| } else if (req.method !== "GET" && req.method !== "HEAD") { | ||
| requestOptions.body = req; | ||
| requestOptions.duplex = "half"; |
There was a problem hiding this comment.
The duplex: "half" option is set unconditionally for non-GET/HEAD requests. This is required for streaming request bodies in fetch, but the TypeScript type definition for RequestInit might not include this property. Consider adding a type assertion or ensuring the types are properly extended to avoid potential type errors:
requestOptions.duplex = "half" as any;| requestOptions.duplex = "half"; | |
| requestOptions.duplex = "half" as any; |
| } | ||
|
|
||
| try { | ||
| const response = await fetch(new URL(outgoingOptions.url).origin + outgoingOptions.path, requestOptions); |
There was a problem hiding this comment.
The fetch function is used without being imported. In Node.js, fetch is available as a global since Node 18, but for better compatibility and explicitness, it should be imported. Consider adding:
import { fetch } from "undici";or use the native fetch explicitly.
| rawHeaders: Object.entries(response.headers).flatMap(([key, value]) => { | ||
| if (Array.isArray(value)) { | ||
| return value.flatMap((v) => (v != null ? [key, v] : [])); | ||
| } | ||
| return value != null ? [key, value] : []; | ||
| }) as string[], |
There was a problem hiding this comment.
The rawHeaders array construction uses Object.entries(response.headers) which doesn't preserve the original header case and order that the HTTP/2 response would have. The Headers object from fetch doesn't provide access to raw headers in the same way Node.js does. Consider documenting this limitation or finding an alternative approach if preserving exact header case/order is important for the proxyRes event consumers.
| onAfterResponse: async (response, _req, _res, _options) => { | ||
| onAfterResponseCalled = true; | ||
| capturedResponse = response; | ||
| console.log(`Response received: ${response.status}`); |
There was a problem hiding this comment.
The console.log statement should be removed from the test code as it adds unnecessary output during test execution. If logging is needed for debugging, consider using a proper test debugging approach or conditional logging.
| console.log(`Response received: ${response.status}`); |
| // Pass your custom fetch implementation | ||
| customFetch, | ||
| onBeforeRequest: async (requestOptions, req, res, options) => { | ||
| requestOptions.headers['X-Custom'] = 'value'; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The documentation shows customFetch being used in the example, but this option is not implemented in the actual code (the stream2 function always uses the global fetch). Either implement support for this option or remove it from the documentation to avoid confusion.
| } | ||
|
|
||
| try { | ||
| const result = await fetch(new URL(outgoingOptions.url).origin + outgoingOptions.path, requestOptions); |
There was a problem hiding this comment.
The fetch function is used without being imported. In Node.js, fetch is available as a global since Node 18, but for better compatibility and explicitness, it should be imported. Consider adding:
import { fetch } from "undici";or use the native fetch explicitly.
|
So it seems like there is something funny with customFetch versus global fetch -- the other comments from AI are mostly points for future work. |
3 participants
This is a proof of concept to explore HTTP/2 proxy support options. The ultimate goal is enabling full HTTP/2 proxy support, which presents a significant challenge: we currently proxy through HTTP/HTTPS requests, but can't detect HTTP/2 support beforehand. This would require attempting HTTP/2 first (using a different API), then falling back to regular requests - adding considerable complexity.
I explored using the Undici client instead, which offers:
To my surprise the code already passes most of the http and websocket tests (but struggling on lib tests).
This approach has significant API compatibility issues:
No custom agent support - HTTP/HTTPS agents are no longer used, breaking existing customizations.
Event model changes - The
proxyReqevent no longer exists, though similar functionality could be provided via interceptors. This is the most critical issue since many integrations depend on these events.The latter point is where this approach falls together, because lots of things are built around these events. But the same would be somewhat true if we use a http2 client request, there is no API compatible way to handle both http2 requests.
So I am somewhat unsure how to proceed from here. First of all as I understand it http-proxy-3 should be a drop-in replacement for http-proxy, so any API changes should probably be forbidden at this point (especially given the rather low adaptation level currently). This leaves some option to consider
So I think the path forward depends on the general path forward and current use cases for this library, which I cannot evaluate.