fix: sse error handling #107
Conversation
to prevent instances where we try to reconnect forever and spam the server.
fix: bug where client crashes when http error code received. chore: lint errors
luxscious
changed the title
There was a problem hiding this comment.
Pull request overview
Improves SSE robustness by propagating read-loop failures to the error handler and introducing an exponential backoff strategy for SSE reconnection.
Changes:
- Wraps
SSEClient.start()and the read loop intry/except/finally, forwarding failures to the configured error handler. - Adds exponential backoff + delayed reconnection thread orchestration in
EnvironmentConfigManager.sse_error. - Fixes
use_new_configboolean precedence for correct SSE URL change detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| devcycle_python_sdk/managers/sse_manager.py | Wraps SSE read loop in error handling and forwards exceptions to the error callback; fixes URL-change condition precedence. |
| devcycle_python_sdk/managers/config_manager.py | Adds exponential backoff reconnection logic and SSE connection recreation/teardown handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@JamieSinn I've opened a new pull request, #108, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update test_config_manager.py
luxscious
force-pushed
the
bug-sse-connection-dropping-after-premature-response-end
branch
from
08cd028 to
4baf61e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def close(self): | ||
| self._polling_enabled = False | ||
| if self._sse_manager is not None and self._sse_manager.client is not None: | ||
| self._sse_manager.client.close() |
There was a problem hiding this comment.
close() currently only closes the SSE client, but it does not stop/join the SSE read thread (SSEManager.read_thread) and does not clear reconnection state. This can leave background threads running after close(). Consider joining the read thread with a timeout and preventing any further reconnect attempts as part of shutdown.
luxscious
deleted the
bug-sse-connection-dropping-after-premature-response-end
branch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| logger.debug(f"DevCycle: failed to read SSE message: {e}") | ||
| logger.debug(f"DevCycle SSE: Error in read loop: {e}") | ||
| fault_event = ld_eventsource.actions.Fault(error=e) | ||
| handle_error(fault_event) | ||
| finally: |
There was a problem hiding this comment.
read_events() now converts any exception into a Fault and calls handle_error(). If the exception is caused by an intentional shutdown (client.close()), this will trigger the reconnection logic and can fight against EnvironmentConfigManager.close(). Consider detecting/ignoring expected shutdown exceptions (or relying on a shutdown flag in the error handler to suppress reconnects during close).
feat: add exponential backoff strategy for SSE reconnection
fix: wrap client.start() in try catch