-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: use axios adapter for OpenAI SDK to support SOCKS5 proxies #7006
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,130 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
import axios, { AxiosRequestConfig, AxiosResponse } from "axios" | ||||||||||||||||||||||||||||||||||||||||||||||
import { Readable } from "stream" | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||
* Creates a fetch-compatible wrapper around axios for use with OpenAI SDK. | ||||||||||||||||||||||||||||||||||||||||||||||
* This adapter allows axios to be used instead of the native fetch API, | ||||||||||||||||||||||||||||||||||||||||||||||
* which is important for proxy support in VSCode extensions where the | ||||||||||||||||||||||||||||||||||||||||||||||
* patched fetch may not work correctly with certain proxy configurations | ||||||||||||||||||||||||||||||||||||||||||||||
* (particularly SOCKS5 proxies). | ||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||
* @param useAxiosForProxy - If true, uses axios instead of native fetch | ||||||||||||||||||||||||||||||||||||||||||||||
* @returns A fetch-compatible function | ||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||
export function createAxiosFetchAdapter(useAxiosForProxy: boolean = false): typeof fetch { | ||||||||||||||||||||||||||||||||||||||||||||||
// If not using axios for proxy, return native fetch | ||||||||||||||||||||||||||||||||||||||||||||||
if (!useAxiosForProxy) { | ||||||||||||||||||||||||||||||||||||||||||||||
return fetch | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Return an axios-based fetch implementation | ||||||||||||||||||||||||||||||||||||||||||||||
return async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { | ||||||||||||||||||||||||||||||||||||||||||||||
const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : (input as Request).url | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When input is a Request object, we're not handling the case where the Request might have a body that needs to be properly extracted. Could we add handling for this case?
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Convert RequestInit to AxiosRequestConfig | ||||||||||||||||||||||||||||||||||||||||||||||
const config: AxiosRequestConfig = { | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The axios adapter doesn't respect the timeout setting that's passed to the OpenAI client constructor. Consider adding timeout configuration to the AxiosRequestConfig:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
url, | ||||||||||||||||||||||||||||||||||||||||||||||
method: (init?.method || "GET") as any, | ||||||||||||||||||||||||||||||||||||||||||||||
headers: init?.headers as any, | ||||||||||||||||||||||||||||||||||||||||||||||
data: init?.body, | ||||||||||||||||||||||||||||||||||||||||||||||
// Important: Set responseType to 'stream' for streaming responses | ||||||||||||||||||||||||||||||||||||||||||||||
responseType: "stream", | ||||||||||||||||||||||||||||||||||||||||||||||
// Disable automatic decompression to let the consumer handle it | ||||||||||||||||||||||||||||||||||||||||||||||
decompress: false, | ||||||||||||||||||||||||||||||||||||||||||||||
// Don't throw on HTTP error status codes | ||||||||||||||||||||||||||||||||||||||||||||||
validateStatus: () => true, | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||||
const axiosResponse: AxiosResponse<Readable> = await axios(config) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Convert axios response to fetch Response | ||||||||||||||||||||||||||||||||||||||||||||||
return createResponseFromAxios(axiosResponse) | ||||||||||||||||||||||||||||||||||||||||||||||
} catch (error: any) { | ||||||||||||||||||||||||||||||||||||||||||||||
// Handle network errors | ||||||||||||||||||||||||||||||||||||||||||||||
throw new TypeError(`Failed to fetch: ${error.message}`) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||
* Converts an Axios response to a fetch Response object | ||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||
function createResponseFromAxios(axiosResponse: AxiosResponse<Readable>): Response { | ||||||||||||||||||||||||||||||||||||||||||||||
const { status, statusText, headers, data } = axiosResponse | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Convert Node.js Readable stream to Web ReadableStream | ||||||||||||||||||||||||||||||||||||||||||||||
const readableStream = nodeStreamToWebStream(data) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// Create Response with proper headers | ||||||||||||||||||||||||||||||||||||||||||||||
const responseHeaders = new Headers() | ||||||||||||||||||||||||||||||||||||||||||||||
Object.entries(headers).forEach(([key, value]) => { | ||||||||||||||||||||||||||||||||||||||||||||||
if (value !== undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
responseHeaders.set(key, String(value)) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
return new Response(readableStream, { | ||||||||||||||||||||||||||||||||||||||||||||||
status, | ||||||||||||||||||||||||||||||||||||||||||||||
statusText, | ||||||||||||||||||||||||||||||||||||||||||||||
headers: responseHeaders, | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||
* Converts a Node.js Readable stream to a Web ReadableStream | ||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||
function nodeStreamToWebStream(nodeStream: Readable): ReadableStream<Uint8Array> { | ||||||||||||||||||||||||||||||||||||||||||||||
return new ReadableStream({ | ||||||||||||||||||||||||||||||||||||||||||||||
start(controller) { | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentional? The Node.js stream event listeners are never removed if the ReadableStream is abandoned before completion. This could cause memory leaks. Consider adding cleanup logic:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
nodeStream.on("data", (chunk) => { | ||||||||||||||||||||||||||||||||||||||||||||||
// Ensure chunk is a Uint8Array | ||||||||||||||||||||||||||||||||||||||||||||||
if (typeof chunk === "string") { | ||||||||||||||||||||||||||||||||||||||||||||||
controller.enqueue(new TextEncoder().encode(chunk)) | ||||||||||||||||||||||||||||||||||||||||||||||
} else if (chunk instanceof Buffer) { | ||||||||||||||||||||||||||||||||||||||||||||||
controller.enqueue(new Uint8Array(chunk)) | ||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||
controller.enqueue(chunk) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
nodeStream.on("end", () => { | ||||||||||||||||||||||||||||||||||||||||||||||
controller.close() | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
nodeStream.on("error", (err) => { | ||||||||||||||||||||||||||||||||||||||||||||||
controller.error(err) | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||
cancel() { | ||||||||||||||||||||||||||||||||||||||||||||||
nodeStream.destroy() | ||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||
* Checks if the current environment suggests that axios should be used | ||||||||||||||||||||||||||||||||||||||||||||||
* instead of fetch for proxy support. This can be based on: | ||||||||||||||||||||||||||||||||||||||||||||||
* - Presence of proxy environment variables | ||||||||||||||||||||||||||||||||||||||||||||||
* - VSCode proxy settings | ||||||||||||||||||||||||||||||||||||||||||||||
* - User configuration | ||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||
export function shouldUseAxiosForProxy(): boolean { | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The proxy detection only checks environment variables but doesn't check VSCode's actual proxy settings. Could we enhance this to also check vscode.workspace.getConfiguration('http').get('proxy')? This would make the detection more comprehensive. |
||||||||||||||||||||||||||||||||||||||||||||||
// Check for common proxy environment variables | ||||||||||||||||||||||||||||||||||||||||||||||
const proxyVars = [ | ||||||||||||||||||||||||||||||||||||||||||||||
"HTTP_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||
"http_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||
"HTTPS_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||
"https_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||
"ALL_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||
"all_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||
"NO_PROXY", | ||||||||||||||||||||||||||||||||||||||||||||||
"no_proxy", | ||||||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
const hasProxyEnvVars = proxyVars.some((varName) => process.env[varName]) | ||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shouldUseAxiosForProxy() function is called on every OpenAI handler instantiation. Could we consider caching this check for performance, especially since environment variables don't change during runtime? |
||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
// For now, we'll enable axios for proxy support if proxy env vars are detected | ||||||||||||||||||||||||||||||||||||||||||||||
// This can be extended to check VSCode settings or user preferences | ||||||||||||||||||||||||||||||||||||||||||||||
return hasProxyEnvVars | ||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
This new file lacks dedicated unit tests. Would it be worth adding tests for stream conversion, error handling, and proxy detection to ensure the adapter works correctly in all scenarios?