fix: ensure streaming requests have default timeout when request_timeout is None#1227
fix: ensure streaming requests have default timeout when request_timeout is None#1227YouFoxGirl wants to merge 1 commit intoe2b-dev:mainfrom
Conversation
…out is None When calling streaming RPC methods (e.g. commands.run) without an explicit request_timeout, the request would hang indefinitely if the sandbox became unreachable - because only connect/pool timeouts were set, not read/write. The fix makes streaming requests fall back to the per-command timeout parameter (which defaults to 60s) for connect/pool/write when request_timeout is not set. This matches the behavior of unary RPC calls which already set all four timeout types. Fixes e2b-dev#1128
YouFoxGirl
requested review from
ValentaTomas,
jakubno and
mishushakov
as code owners
|
![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: 2a5343f7e1
ℹ️ 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".
| timeout_ext["connect"] = request_timeout | ||
| timeout_ext["pool"] = request_timeout | ||
| timeout_ext["write"] = request_timeout | ||
| elif timeout is not None: |
There was a problem hiding this comment.
This new fallback runs whenever request_timeout is None, including when callers pass timeout=0 (documented elsewhere in the SDK as “no limit”). In that case this branch now sets connect/pool/write to 0, which in httpcore/httpx timeout extensions is treated as an immediate timeout rather than unlimited, so streaming calls can fail instantly instead of remaining unbounded. The regression occurs when users disable request timeouts (request_timeout=0/None) and rely on timeout=0 for long-lived streams.
Useful? React with 👍 / 👎.
| client = Client( | ||
| url="http://localhost:8080", | ||
| response_type=MagicMock, | ||
| json=True, |
There was a problem hiding this comment.
This test configures Client(..., json=True), so _prepare_server_stream_request uses JSONCodec.encode (json_format.MessageToJson), which expects a real protobuf message object with protobuf metadata (e.g. DESCRIPTOR). FakeMsg only defines SerializeToString, so the test will raise before reaching the timeout assertions once protobuf/http deps are present, making the regression test ineffective.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #1128 — streaming requests hang indefinitely when sandbox is unreachable
When calling streaming RPC methods (e.g.
commands.run) without an explicitrequest_timeout, the request would hang indefinitely if the sandbox became unreachable - because onlyconnect/pooltimeouts were set, notread/write.The fix makes streaming requests fall back to the per-command
timeoutparameter (which defaults to 60s) forconnect/pool/writewhenrequest_timeoutis not set. This matches the behavior of unary RPC calls which already set all four timeout types.Changes
In
e2b_connect/client.py,_prepare_server_stream_request:request_timeoutisNone(the default), fall back totimeoutforconnect/pool/writetimeoutsrequest_timeoutis explicitly provided, it still takes precedence (existing behavior)Added unit test verifying both cases.