Refactor Connection Config #97
Conversation
ninan-nn
force-pushed
the
hotfix/fix_random_peer_connection_closed
branch
4 times, most recently
from
b76e0e5 to
381cf3a
Compare
|
update readme about js sdk under project root |
ninan-nn
force-pushed
the
hotfix/fix_random_peer_connection_closed
branch
3 times, most recently
from
33b95c0 to
a11a972
Compare
Pangjiping
added
the
approved
ninan-nn
force-pushed
the
hotfix/fix_random_peer_connection_closed
branch
2 times, most recently
from
07d99ce to
48f875f
Compare
ninan-nn
force-pushed
the
hotfix/fix_random_peer_connection_closed
branch
from
48f875f to
1bec8a7
Compare
|
@codex review |
![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.
Reviewed commit: 1bec8a7e83
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dispatcher = new Agent({ | ||
| keepAliveTimeout: DEFAULT_KEEPALIVE_TIMEOUT_MS, | ||
| keepAliveMaxTimeout: DEFAULT_KEEPALIVE_TIMEOUT_MS, | ||
| }); |
There was a problem hiding this comment.
The new createNodeFetch allocates a dedicated undici Agent per ConnectionConfig, but the SDK doesn’t expose any way to close/destroy that agent. In long‑lived Node processes that create and discard many Sandbox/Manager instances, these per‑config pools can leave sockets/timers around until process exit. This is a new resource‑leak risk compared to using the global fetch. Consider wiring a close hook (e.g., on manager/sandbox close) or reusing a shared agent so these pools can be released.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function normalizeDomainBase(input: string): { protocol?: ConnectionProtocol; domainBase: string } { | ||
| const DEFAULT_KEEPALIVE_TIMEOUT_MS = 15_000; |
There was a problem hiding this comment.
Sure about this default value? It's not consistent with SDK default keep-alive is 30 seconds for its own pools. mentioned in REAME.md
| private val logger = LoggerFactory.getLogger(HttpClientProvider::class.java) | ||
|
|
||
| private val connectionPool = | ||
| config.connectionPool ?: ConnectionPool(32, 15, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Better to extract 15 to constant and also has the same inconsistency problem with README.md
4 participants
Summary
Ensured separate connection pools are allocated for each sandbox/manager instance, even when using a singleton ConnectionConfig.
Updated several connection pool settings.
Testing
Breaking Changes
Checklist