-
Notifications
You must be signed in to change notification settings - Fork 6
fix: add proper exception handling for background event source #281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a safety wrapper around BackgroundEventSource to prevent crashes caused by RejectedExecutionException during start/close lifecycle. Key changes include introducing SafeBackgroundEventSource with guarded start/close methods and updating DevCycleClient to use the new wrapper.
- Introduces SafeBackgroundEventSource with explicit exception handling
- Replaces direct BackgroundEventSource usage in DevCycleClient with the safe wrapper
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| SafeBackgroundEventSource.kt | New wrapper adds guarded start/close to avoid executor rejection crashes |
| DevCycleClient.kt | Replaces raw BackgroundEventSource with SafeBackgroundEventSource to apply safer lifecycle management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...d-client-sdk/src/main/java/com/devcycle/sdk/android/eventsource/SafeBackgroundEventSource.kt
Outdated
Show resolved
Hide resolved
...d-client-sdk/src/main/java/com/devcycle/sdk/android/eventsource/SafeBackgroundEventSource.kt
Outdated
Show resolved
Hide resolved
...d-client-sdk/src/main/java/com/devcycle/sdk/android/eventsource/SafeBackgroundEventSource.kt
Outdated
Show resolved
Hide resolved
...d-client-sdk/src/main/java/com/devcycle/sdk/android/eventsource/SafeBackgroundEventSource.kt
Outdated
Show resolved
Hide resolved
...d-client-sdk/src/main/java/com/devcycle/sdk/android/eventsource/SafeBackgroundEventSource.kt
Outdated
Show resolved
Hide resolved
jsalaber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can also make use of the options in the EventSource to always continue instead of throwing an exception
3ba64bf to
3b233c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ConnectStrategy.http(URI(config?.sse?.url)) | ||
| .readTimeout(EVENT_SOURCE_RETRY_DELAY_MIN, TimeUnit.MINUTES) | ||
| ) | ||
| ).errorStrategy(ErrorStrategy.alwaysContinue()) |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ErrorStrategy.alwaysContinue() silently suppresses all errors, which could mask critical issues and make debugging difficult. Consider implementing a custom error strategy that logs errors before continuing, or use a strategy that retries with backoff for transient errors while failing fast for non-recoverable ones.
| ).errorStrategy(ErrorStrategy.alwaysContinue()) | |
| ).errorStrategy { error, _ -> | |
| DevCycleLogger.e("EventSource error: ${error.message}", error) | |
| ErrorStrategy.Action.CONTINUE | |
| } |
Uh oh!
There was an error while loading. Please reload this page.