-
Notifications
You must be signed in to change notification settings - Fork 39
feat: Replace agentkeepalive with native Node.js HTTP agents for proxy support
#788
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import http from 'node:http'; | ||
| import https from 'node:https'; | ||
| import os from 'node:os'; | ||
|
|
||
| import KeepAliveAgent from 'agentkeepalive'; | ||
| import type { RetryFunction } from 'async-retry'; | ||
| import retry from 'async-retry'; | ||
| import type { AxiosError, AxiosInstance, AxiosRequestConfig, AxiosResponse, InternalAxiosRequestConfig } from 'axios'; | ||
|
|
@@ -32,9 +33,9 @@ export class HttpClient { | |
|
|
||
| timeoutMillis: number; | ||
|
|
||
| httpAgent?: KeepAliveAgent; | ||
| httpAgent?: http.Agent; | ||
|
|
||
| httpsAgent?: KeepAliveAgent.HttpsAgent; | ||
| httpsAgent?: https.Agent; | ||
|
|
||
| axios: AxiosInstance; | ||
|
|
||
|
|
@@ -53,16 +54,37 @@ export class HttpClient { | |
|
|
||
| if (isNode()) { | ||
| // We want to keep sockets alive for better performance. | ||
| // It's important to set the user's timeout also here and not only | ||
| // on the axios instance, because even though this timeout | ||
| // is for inactive sockets, sometimes the platform would take | ||
| // long to process requests and the socket would time-out | ||
| // while waiting for the response. | ||
| const agentOpts = { | ||
| // Enhanced agent configuration based on agentkeepalive best practices: | ||
| // - Nagle's algorithm disabled for lower latency | ||
| // - Free socket timeout to prevent socket leaks | ||
| // - LIFO scheduling to reuse recent sockets | ||
| // - Socket TTL for connection freshness | ||
| const agentOptions: http.AgentOptions & { scheduling?: 'lifo' | 'fifo' } = { | ||
| keepAlive: true, | ||
|
Contributor
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.
Contributor
Author
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. Good point, I've just added commit c424e90 with these optimizations. |
||
| // Timeout for inactive sockets | ||
| // Prevents socket leaks from idle connections | ||
| timeout: this.timeoutMillis, | ||
| // Keep alive timeout for free sockets (15 seconds) | ||
| // Node.js will close unused sockets after this period | ||
| keepAliveMsecs: 15_000, | ||
| // Maximum number of sockets per host | ||
| maxSockets: 256, | ||
| maxFreeSockets: 256, | ||
| // LIFO scheduling - reuse most recently used sockets for better performance | ||
| scheduling: 'lifo', | ||
| }; | ||
| this.httpAgent = new KeepAliveAgent(agentOpts); | ||
| this.httpsAgent = new KeepAliveAgent.HttpsAgent(agentOpts); | ||
|
|
||
| this.httpAgent = new http.Agent(agentOptions); | ||
| this.httpsAgent = new https.Agent(agentOptions); | ||
|
|
||
| // Disable Nagle's algorithm for lower latency | ||
| // This sends data immediately instead of buffering small packets | ||
| const setNoDelay = (socket: any) => { | ||
| socket.setNoDelay(true); | ||
| }; | ||
|
|
||
| this.httpAgent.on('socket', setNoDelay); | ||
| this.httpsAgent.on('socket', setNoDelay); | ||
| } | ||
|
|
||
| this.axios = axios.create({ | ||
|
|
||

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.
Can you please add both these modules as
externalstorsbuild.config.ts(link)? They are not used outside of Node, so we don't need them in the browser bundle. Together, they add around 250kB to the bundle size.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.
Sure thing, done in commit f986aa4
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.
Thank you!