SocketException: Reading from a closed socket crash#4526
SocketException: Reading from a closed socket crash#4526
Conversation
… crash Return a synthetic 503 StreamedResponse when a SocketException occurs in sendStreaming instead of letting the exception propagate as a fatal crash. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a crash caused by an unhandled SocketException in sendStreaming by catching the exception and returning a synthetic 503 response. This is a good approach to gracefully handle network interruptions during streaming requests. My review includes one suggestion to improve observability by logging the caught exception, which is important for monitoring since these errors will no longer appear as crashes.
| }) async { | ||
| try { | ||
| return await _client.send(request).timeout(timeout); | ||
| } on SocketException { |
There was a problem hiding this comment.
While this change correctly prevents the crash, it silently swallows the SocketException. This means you will lose visibility into these network errors, which were previously reported as crashes. It would be beneficial to log the exception to aid in future debugging and monitoring.
First, capture the exception object. Then, you can log it using a suitable logging utility from your project (e.g., DebugLogManager.logWarning).
on SocketException catch (e, stackTrace) {
// You may need to import your logging utility
DebugLogManager.logWarning('send_streaming_socket_error', {
'error': e.toString(),
'stackTrace': stackTrace.toString(),
'request_url': request.url.toString(),
});
// ... rest of the block
}| } on SocketException { | |
| } on SocketException catch (e, stackTrace) { |
|
Hey @aaravgarg! Thanks for this SocketException crash fix — large impact (3,021 events, 533 users). We're tidying up stale PRs — this one has been open 8 days with untested test plan and no demo. The gemini review noted the fix silently swallows the exception, which could hide issues. Could you run the test plan, consider the review feedback, and add a demo? Feel free to reopen once ready! |
|
Hey @aaravgarg 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
Summary
sendStreamingwhen aSocketException("Reading from a closed socket") is thrownStreamedResponseinstead of letting the exception propagate as a fatal crashCrash Stats
Test plan
🤖 Generated with Claude Code