[CDX-358] Handle undeliverable RxJava exceptions

[HHHindawy]

Summary:

• Add a global RxJavaPlugins.setErrorHandler to catch undeliverable RxJava exceptions (e.g. network errors that occur after a stream is disposed) and log them instead of crashing the host app
• Change Retrofit's RxJava2CallAdapterFactory.create() to createWithScheduler(Schedulers.io()) to ensure network calls are consistently subscribed on the IO scheduler, reducing the likelihood of undeliverable exceptions
• Add unit tests to verify the error handler gracefully handles IOException, InterruptedException, unexpected exceptions, and null cause edge cases

Background:

OkHttp can deliver exceptions to RxJava after a stream has already completed or been disposed. When no global error handler is set, RxJava's default behavior is to throw these as unhandled exceptions, which crashes the host app. This is especially problematic for an SDK where internal network errors should never bring down the consumer's application.

Why createWithScheduler(Schedulers.io())?

With RxJava2CallAdapterFactory.create(), Retrofit observables don't have a default subscription scheduler, meaning the thread that subscribes is the thread that executes the network call. If a subscriber disposes while OkHttp is mid-request on an uncontrolled thread, the resulting error has no downstream observer and becomes an undeliverable exception. By using createWithScheduler(Schedulers.io()), all Retrofit calls are consistently subscribed on the RxJava IO scheduler, which works cooperatively with RxJava's disposal and error-handling mechanisms, significantly reducing the window for undeliverable exceptions.

[Copilot]

Pull request overview

This PR aims to prevent host-app crashes caused by RxJava “undeliverable” exceptions (often originating from OkHttp/Retrofit calls after disposal) by installing a global RxJava error handler and standardizing Retrofit Rx subscriptions onto the IO scheduler.

Changes:

• Install a global RxJavaPlugins.setErrorHandler during ConstructorIo.init() to handle/log undeliverable exceptions.
• Configure Retrofit to use RxJava2CallAdapterFactory.createWithScheduler(Schedulers.io()) so subscriptions default to the IO scheduler.
• Add unit tests that exercise the error handler against several edge-case exception types.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
library/src/main/java/io/constructor/core/ConstructorIo.kt	Adds/installs a global RxJava error handler during SDK initialization.
library/src/main/java/io/constructor/injection/module/NetworkModule.kt	Switches Retrofit Rx adapter factory to use an IO scheduler by default.
library/src/test/java/io/constructor/core/ConstructorIoRxErrorHandlerTest.kt	Adds unit tests to ensure the handler doesn’t throw for key exception cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[esezen]

Looking great. I left some comments

[constructor-claude-bedrock]

Code Review Summary

This PR addresses a real and well-documented RxJava problem: undeliverable exceptions crashing the host app when network errors arrive after stream disposal. The overall approach is sound and the PR description is excellent. The implementation has a few issues worth addressing before merging, ranging from a correctness concern to minor nits.

---

Detailed Feedback

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Line: L146-L147] @Synchronized is incorrect here and provides false safety

The @Synchronized annotation locks on the ConstructorIo object instance (the singleton's monitor). However, RxJavaPlugins.getErrorHandler() and RxJavaPlugins.setErrorHandler() are static/global operations that are not protected by that same lock. This means two threads using different locks (or no lock at all) can still race. The guard if (RxJavaPlugins.getErrorHandler() != null) return is a check-then-act race condition regardless of the lock.

In practice this is low-risk since init is called once at app startup, but the annotation creates a misleading sense of correctness. Consider removing @Synchronized and documenting that init is expected to be called from a single thread, or use AtomicBoolean for a truly safe once-only guard:

private val errorHandlerInstalled = AtomicBoolean(false)

internal fun setupRxJavaErrorHandler() {
    if (errorHandlerInstalled.getAndSet(true)) return
    if (RxJavaPlugins.getErrorHandler() != null) return
    RxJavaPlugins.setErrorHandler { throwable -> ... }
}

---

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Lines: L160, L167, L179] Using e() (error level) for all log entries

The e() helper maps to Log.e(). All three log statements use error level, including non-fatal network exceptions (timeouts, no connectivity) that are completely routine during normal operation. These will appear as errors in logcat and any crash/log monitoring tools the host app uses. Consider using d() (debug) for the IOException path since those are expected events, reserving e() for the unexpected exception fallback path.

---

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Lines: L171-L180] No logging before forwarding unexpected exceptions to the uncaught handler

For unexpected (non-IO, non-Interrupted) exceptions, the handler calls handler.uncaughtException(thread, error). If a custom crash-reporting SDK or test framework has installed a handler that suppresses the exception, there will be no Constructor.io-specific trace in the logs. Consider adding a log line before forwarding:

e("Constructor.io: Forwarding unexpected undeliverable exception to uncaught handler: $error")
handler.uncaughtException(Thread.currentThread(), error)

---

[File: library/src/test/java/io/constructor/core/ConstructorIoRxErrorHandlerTest.kt Lines: L72-L87] Missing assertNotNull before assertSame in forwardsUnexpectedExceptionToUncaughtExceptionHandler

If a bug caused the handler to never be invoked, caughtThrowable would remain null and assertSame(error, caughtThrowable) would produce a confusing failure message. Adding assertNotNull first gives a clearer signal:

assertNotNull("Uncaught exception handler was never invoked", caughtThrowable)
assertSame(error, caughtThrowable)

---

[File: library/src/test/java/io/constructor/core/ConstructorIoRxErrorHandlerTest.kt] Missing test: raw (non-wrapped) InterruptedException

The test suite covers a raw (non-wrapped) IOException via handlesRawIOException but has no equivalent for a raw InterruptedException. For consistency, consider adding:

@Test
fun handlesRawInterruptedException() {
    Thread.interrupted() // clear any pre-existing flag
    RxJavaPlugins.onError(InterruptedException("raw interrupted"))
    assertTrue("Interrupt flag should be restored", Thread.interrupted())
}

---

[General] createWithScheduler(Schedulers.io()) change deserves an inline comment in NetworkModule.kt

The change from RxJava2CallAdapterFactory.create() to createWithScheduler(Schedulers.io()) is correct and well-motivated. However, the rationale is invisible at the call site. A short comment would protect this from being reverted as unnecessary:

// Use createWithScheduler so Retrofit observables always subscribe on the IO scheduler,
// reducing the window for undeliverable exceptions when subscribers dispose mid-request.
.addCallAdapterFactory(RxJava2CallAdapterFactory.createWithScheduler(Schedulers.io()))

---

Conclusion

The PR solves a genuine SDK stability issue with a well-structured approach and solid test coverage. The most significant concern is @Synchronized providing misleading thread-safety guarantees on what is effectively a global static operation. The logging level inconsistency and missing call-site comment are minor quality improvements. The test suite is good overall — the missing raw InterruptedException test and the assertNotNull improvement are low-priority but would make it more robust. Overall this is good work and close to merge-ready.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[esezen]

LGTM!