Fetch refinements #42
Conversation
|
finally resolved the websocket (not just test) errors. Just a few more tests error before fetch path passes all tests (except where not applicable) |
primarily look for x-forwarded-host instead of host header
Corvince
force-pushed
the
fetch-refinements
branch
from
b35994b to
3550ee4
Compare
|
All tests passing :) There were some remaining tests failing, which was due to the test expecting the "host" header to contain the port number of the proxy - something fetch doesnt allow you to do (the host header is forbidden do set manually and must always represent the actual host the request is made to. Additionally for h2, the host header doesn't even exist. The way I solved it was to add Now the only tests that are being skipped relate to the not supported proxyReq and proxyRes events, the preserveHeaderKeyCase (also not supported) and the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (options.proxyTimeout) { | ||
| setTimeout(() => { | ||
| controller.abort(); | ||
| }, options.proxyTimeout); | ||
| } |
There was a problem hiding this comment.
In the fetch code path a setTimeout now calls controller.abort() after options.proxyTimeout elapses, but the timer is never cleared or coupled to socket inactivity. As a result any fetch request that legitimately streams longer than the configured timeout (e.g., large downloads) will always be aborted and reported as ECONNRESET, whereas the native path uses proxyReq.setTimeout which only fires when the socket is idle. This regresses long-running fetch-based proxies whenever proxyTimeout is set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That does seem like a good point. Uncancelled setTimeout's are a red flag.
There was a problem hiding this comment.
I agree. Honestly the timeout handling is what copilot came up with after I asked it to improve error handling - not sure why it was added, but looked good enough (especially adding the AbortController seemed sensible). But yes, I will look into this
There was a problem hiding this comment.
I removed the abortcontroller and just added a AbortSignal.timeout if proxyTimeout is set. This still cancels the request independent of it being still active, but actually chatgpt is wrong here and this is also what happens in the native path.
This also simplifies the code
2 participants
Some small improvements and additions to the fetch path. Now additionally runs and passes all tests with the fetch path