-
Notifications
You must be signed in to change notification settings - Fork 10
Fetch refinements #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
84288f0
improve fetch documentation
89e05af
derive x-forwarded-host from :authority for http2
59aed83
unify request preparation between forward and proxy
c9f0023
update error handling in stream2, add abort controller
ad74f5a
re-enable fetch test
3c6e4fe
fix ts error
9b395e6
replace ws with http for ws inital connections
3550ee4
test fixes
625bd8a
clarify some missing options in fetch
301bc51
remove AbortController in favor of timeout AbortSignal
11180ed
update message
50f0722
listen to correct timeout code
d24f40f
Merge branch 'main' of https://github.com/sagemathinc/http-proxy-3 in…
fb178e1
use xfwd host to test succesfull usage of proxy
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| pnpm test proxy-http2-to-http2.test.ts | ||
| pnpm test customFetch.test.ts | ||
|
|
||
| */ | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
|
|
||
| import * as http from "node:http"; | ||
| import * as httpProxy from "../.."; | ||
| import getPort from "../get-port"; | ||
| import { describe, it, expect, beforeAll, afterAll } from "vitest"; | ||
| import { fetch } from "undici"; | ||
|
|
||
| describe("Fetch Proxy Timeout", () => { | ||
| let ports: Record<"http" | "proxy", number>; | ||
| beforeAll(async () => { | ||
| ports = { http: await getPort(), proxy: await getPort() }; | ||
| }); | ||
|
|
||
| const servers: Record<string, any> = {}; | ||
|
|
||
| it("Create the target HTTP server that hangs", async () => { | ||
| servers.http = http | ||
| .createServer((_req, _res) => { | ||
| // Do nothing, let it hang | ||
| }) | ||
| .listen(ports.http); | ||
| }); | ||
|
|
||
| it("Create the proxy server with fetch and timeout", async () => { | ||
| servers.proxy = httpProxy | ||
| .createServer({ | ||
| target: `http://localhost:${ports.http}`, | ||
| fetch: fetch as any, // Enable fetch path | ||
| proxyTimeout: 500, // 500ms timeout | ||
| }) | ||
| .listen(ports.proxy); | ||
| }); | ||
|
|
||
| it("should timeout the request and emit error", async () => { | ||
| return new Promise<void>((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| reject(new Error("Test timed out")); | ||
| }, 2000); | ||
|
|
||
| servers.proxy.once('error', (err: Error, _req: any, res: any) => { | ||
| clearTimeout(timeout); | ||
| try { | ||
| expect(err).toBeTruthy(); | ||
| expect(err.message).toBe("Proxy timeout"); | ||
| res.statusCode = 504; | ||
| res.end("Gateway Timeout"); | ||
| resolve(); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
|
|
||
| fetch(`http://localhost:${ports.proxy}`).catch(() => { | ||
| // Ignore client side fetch error, we care about server side error emission | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| Object.values(servers).map((x: any) => x?.close()); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
|
|
||
| import * as http from "node:http"; | ||
| import * as httpProxy from "../.."; | ||
| import getPort from "../get-port"; | ||
| import { join } from "node:path"; | ||
| import { readFile } from "node:fs/promises"; | ||
| import { describe, it, expect, beforeAll, afterAll } from "vitest"; | ||
| import { Agent, fetch } from "undici"; | ||
|
|
||
| const TestAgent = new Agent({ allowH2: true, connect: { rejectUnauthorized: false } }); | ||
|
|
||
| const fixturesDir = join(__dirname, "..", "fixtures"); | ||
|
|
||
| describe("X-Forwarded-Host with HTTP/2", () => { | ||
| let ports: Record<"http" | "proxy", number>; | ||
| beforeAll(async () => { | ||
| ports = { http: await getPort(), proxy: await getPort() }; | ||
| }); | ||
|
|
||
| const servers: any = {}; | ||
|
|
||
| it("Create the target HTTP server", async () => { | ||
| servers.http = http | ||
| .createServer((req, res) => { | ||
| res.writeHead(200, { "Content-Type": "application/json" }); | ||
| res.write(JSON.stringify(req.headers)); | ||
| res.end(); | ||
| }) | ||
| .listen(ports.http); | ||
| }); | ||
|
|
||
| it("Create the HTTPS proxy server with xfwd", async () => { | ||
| servers.proxy = httpProxy | ||
| .createServer({ | ||
| target: { | ||
| host: "localhost", | ||
| port: ports.http, | ||
| }, | ||
| ssl: { | ||
| key: await readFile(join(fixturesDir, "agent2-key.pem"), "utf8"), | ||
| cert: await readFile(join(fixturesDir, "agent2-cert.pem"), "utf8"), | ||
| }, | ||
| xfwd: true, | ||
| }) | ||
| .listen(ports.proxy); | ||
| }); | ||
|
|
||
| it("should pass x-forwarded-host when using HTTP/2", async () => { | ||
| const res = await fetch(`https://localhost:${ports.proxy}`, { dispatcher: TestAgent }); | ||
| const headers = await res.json() as any; | ||
|
|
||
| // In HTTP/2, :authority is used instead of Host. | ||
| // The proxy should map :authority to x-forwarded-host if Host is missing. | ||
| expect(headers["x-forwarded-host"]).toBe(`localhost:${ports.proxy}`); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| // cleans up | ||
| Object.values(servers).map((x: any) => x?.close()); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the fetch code path a
setTimeoutnow callscontroller.abort()afteroptions.proxyTimeoutelapses, 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 asECONNRESET, whereas the native path usesproxyReq.setTimeoutwhich only fires when the socket is idle. This regresses long-running fetch-based proxies wheneverproxyTimeoutis set.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does seem like a good point. Uncancelled setTimeout's are a red flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the abortcontroller and just added a
AbortSignal.timeoutif 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