send errors to pending requests if server closes - #333
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for addressing this issue - it will improve the reliability of the Python SDK.
One small improvement is needed: after sending errors and closing the streams, we should clear the _response_streams dictionary to prevent keeping references to closed streams.
# Current implementation in the PR:
for id, stream in self._response_streams.items():
error = ErrorData(code=CONNECTION_CLOSED, message="Connection closed")
await stream.send(JSONRPCError(jsonrpc="2.0", id=id, error=error))
await stream.aclose()
# Suggested implementation:
for id, stream in self._response_streams.items():
error = ErrorData(code=CONNECTION_CLOSED, message="Connection closed")
await stream.send(JSONRPCError(jsonrpc="2.0", id=id, error=error))
await stream.aclose()
self._response_streams.clear() This matches the existing pattern in the codebase where self._response_streams.pop(request_id, None) is used to remove individual streams after use.
With this small change, the PR should be ready to merge.
Sorry, something went wrong.
ihrpr
left a comment
There was a problem hiding this comment.
Thank you!!
Would be great to add
self._response_streams.clear()
before merging.
Sorry, something went wrong.
Motivation and Context
Fixes #332
The client blocks indefinitely if the server streams close before responding to a pending request.
How Has This Been Tested?
Added a test. Without this fix, the test fails because the request blocks forever.
Breaking Changes
No
Types of changes
Checklist