fix: handle SSE connection state when Start events are not received #105
Conversation
luxscious
force-pushed
the
fix-reduce-logging-frequency
branch
from
38e6a23 to
ed5bf3c
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes SSE (Server-Sent Events) connection state handling to ensure the client correctly establishes connection status even when the SSE server sends events in unexpected orders (e.g., Comment or Event messages before Start events).
Key changes:
- Modified
sse_state()to acceptOptional[Start]parameter and prevent duplicate connection logs - Updated
sse_message()to callsse_state(None)as a fallback when events arrive before Start - Added Comment event handling in SSE manager to trigger connection state update
- Increased default config polling interval from 1s to 30s (reasonable when SSE provides real-time updates)
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| devcycle_python_sdk/options.py | Increased default config_polling_interval_ms from 1000 to 30000ms to reduce polling frequency when SSE real-time updates are available |
| devcycle_python_sdk/managers/sse_manager.py | Added handling for Comment events to trigger SSE connection state initialization |
| devcycle_python_sdk/managers/config_manager.py | Updated sse_state() signature to Optional[Start] and modified sse_message() to ensure connection state is set regardless of event order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def sse_state(self, state: Optional[ld_eventsource.actions.Start]): | ||
| # Prevents duplicate logs when Comment events call this repeatedly | ||
| if not self._sse_connected: | ||
| self._sse_connected = True | ||
| logger.info("DevCycle: Connected to SSE stream") |
There was a problem hiding this comment.
The new behavior introduced in sse_state() and sse_message() (handling Comment events and calling sse_state with None) lacks test coverage. Consider adding tests that verify the SSE connection state is correctly set when receiving Comment events before Start events, and when receiving Event messages before Start events.
| self._local_bucketing.store_config(json_config) | ||
| if not self._options.disable_realtime_updates: | ||
| if self._sse_manager is None: | ||
| if self._sse_manager is None or not self._sse_manager.client.is_connected(): |
There was a problem hiding this comment.
does this mean you create a new SSEManager when the client isn't connected? wouldn't it be better to do some sort of reconnection strategy?
There was a problem hiding this comment.
yea if this becomes an issue i think we should revisit this. gonna merge this for now --> Its a pre-emptive solution to an anticipated problem.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 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 sse_state(self, state: ld_eventsource.actions.Start): | ||
| self._sse_connected = True | ||
| logger.info("DevCycle: Connected to SSE stream") | ||
| def sse_state(self, state: Optional[ld_eventsource.actions.Start]): |
There was a problem hiding this comment.
Type signature inconsistency detected. The SSEManager constructor expects handle_state to be of type Callable[[ld_eventsource.actions.Start], None] (line 15 in sse_manager.py), but the sse_state method signature has been changed to accept Optional[ld_eventsource.actions.Start]. The SSEManager type hints should be updated to reflect this change, using Optional[ld_eventsource.actions.Start] instead of just ld_eventsource.actions.Start.
| self._sse_connected = False | ||
| logger.debug(f"DevCycle: Received SSE error: {error}") | ||
|
|
||
| def sse_state(self, state: ld_eventsource.actions.Start): | ||
| self._sse_connected = True | ||
| logger.info("DevCycle: Connected to SSE stream") | ||
| def sse_state(self, state: Optional[ld_eventsource.actions.Start]): | ||
| if not self._sse_connected: | ||
| self._sse_connected = True | ||
| logger.info("DevCycle: Connected to SSE stream") |
There was a problem hiding this comment.
Potential race condition with _sse_connected flag. This boolean flag is accessed and modified from multiple threads without synchronization: the main polling thread (line 128 in run()) and the SSE event handling thread (via sse_message, sse_error, and sse_state callbacks). Consider using a threading.Lock to protect access to _sse_connected, or use a thread-safe alternative like threading.Event.
| if not self._options.disable_realtime_updates: | ||
| if self._sse_manager is None: | ||
| if ( | ||
| self._sse_manager is None |
There was a problem hiding this comment.
Potential AttributeError when accessing self._sse_manager.client.is_connected(). The SSEManager's client attribute is initialized to None in the constructor and only gets assigned when the update() method is called. This check could raise an AttributeError if the SSEManager exists but update() hasn't been called yet. Consider adding a None check for the client before calling is_connected().
| self._sse_manager is None | |
| self._sse_manager is None | |
| or self._sse_manager.client is None |
Event messages before Start events
This ensures _sse_connected is correctly set regardless of whether the SSE
server sends Start events, Comment events, or Event messages first.