[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]

RxJavaPlugins.setErrorHandler can throw IllegalStateException if RxJava plugins have been locked down by the host app (e.g., via RxJavaPlugins.lockdown()), which would make ConstructorIo.init() crash. Consider guarding setErrorHandler with a try/catch and falling back to logging if the handler can't be installed.

Suggested change

	RxJavaPlugins.setErrorHandler { throwable ->
	var error = throwable
	// Unwrap the actual cause from UndeliverableException
	if (error is UndeliverableException) {
	error = error.cause ?: error
	}
	// Network exceptions are expected during normal operation (timeout, no connectivity, etc.)
	// Log them but don't crash the app
	if (error is IOException || error is InterruptedException) {
	e("Constructor.io: Non-fatal network error: ${error.javaClass.simpleName} - ${error.message}")
	return@setErrorHandler
	}
	// For other unexpected exceptions, log them as errors
	// but still don't crash - these shouldn't bring down the host app
	e("Constructor.io: Undeliverable exception: ${error.javaClass.simpleName} - ${error.message}")
	try {
	RxJavaPlugins.setErrorHandler { throwable ->
	var error = throwable
	// Unwrap the actual cause from UndeliverableException
	if (error is UndeliverableException) {
	error = error.cause ?: error
	}
	// Network exceptions are expected during normal operation (timeout, no connectivity, etc.)
	// Log them but don't crash the app
	if (error is IOException || error is InterruptedException) {
	e("Constructor.io: Non-fatal network error: ${error.javaClass.simpleName} - ${error.message}")
	return@setErrorHandler
	}
	// For other unexpected exceptions, log them as errors
	// but still don't crash - these shouldn't bring down the host app
	e("Constructor.io: Undeliverable exception: ${error.javaClass.simpleName} - ${error.message}")
	}
	} catch (ise: IllegalStateException) {
	// RxJava plugins may have been locked down by the host app; fall back to logging only.
	e("Constructor.io: Failed to set RxJava global error handler; plugins may be locked down: ${ise.message}")

[Copilot]

The global error handler currently swallows all undeliverable exceptions by logging and returning. RxJava’s recommended pattern is to forward programmer bugs (e.g., NullPointerException, IllegalArgumentException, IllegalStateException, OnErrorNotImplementedException) to the thread’s UncaughtExceptionHandler (or delegate to any existing error handler) so they aren’t silently ignored in the host app.

[Copilot]

Logging here drops the throwable/stack trace (the imported e() helper only logs a message). For post-mortem debugging, consider logging the Throwable as well (e.g., Log.e(tag, msg, error) or adding an e(msg, throwable) overload) so crash reporters and logcat contain the full stack trace.

[Copilot]

When handling InterruptedException, it’s usually important to restore the thread’s interrupted status (Thread.currentThread().interrupt()) before returning; otherwise downstream code may miss the interrupt signal.

Suggested change

	if (error is IOException || error is InterruptedException) {
	if (error is IOException || error is InterruptedException) {
	if (error is InterruptedException) {
	Thread.currentThread().interrupt()
	}

[constructor-claude-bedrock]

Code Review Summary

PR [CDX-358] adds a global RxJavaPlugins.setErrorHandler to prevent undeliverable RxJava exceptions from crashing the host app, switches Retrofit's call adapter factory to use createWithScheduler(Schedulers.io()), and adds unit tests for the new error handler. The motivation is sound and well-documented. The implementation is clean and follows existing patterns in the codebase. I have a few observations and minor concerns below.

---

Detailed Feedback

[File: ConstructorIo.kt — setupRxJavaErrorHandler()] Idempotency check may have a race condition in theory

The guard:

if (RxJavaPlugins.getErrorHandler() != null) return

is not atomic — two threads calling setupRxJavaErrorHandler() simultaneously could both see null and both call setErrorHandler. In practice this is unlikely because init() should only be called once, but since setupRxJavaErrorHandler() is internal and could be called by tests or third-party code, it is worth noting. The test isIdempotent calls it twice sequentially and passes, but it doesn't cover the concurrent case. This is low-risk for an SDK that documents single-threaded initialization, but worth a comment.

[File: ConstructorIo.kt — setupRxJavaErrorHandler()] internal visibility on a public object

Marking setupRxJavaErrorHandler() as internal is reasonable for test access, but it does expose this function to any code in the same module. If this is intentional (tests only), a KDoc note explaining why it's internal rather than private would help future maintainers. The current KDoc only explains what the function does, not why it needs to be internal.

[File: ConstructorIo.kt — setupRxJavaErrorHandler()] Log tag inconsistency

The e() extension function in Extensions.kt uses this::class.qualifiedName as the tag, which resolves to io.constructor.core.ConstructorIo. However, the existing log messages in the SDK (as seen in other parts of ConstructorIo.kt) consistently prefix messages with "Constructor.io: ..." inline in the message string. The new messages duplicate that prefix. Since the tag already identifies the source, the inline prefix is redundant — but more importantly, it is inconsistent with the broader Android logging convention of keeping the message separate from the tag. This is a style nit, not a bug.

[File: ConstructorIo.kt — setupRxJavaErrorHandler()] Unexpected exceptions are forwarded silently to the uncaught exception handler

For truly unexpected exceptions (not IOException or InterruptedException), the handler forwards to the thread's uncaught exception handler:

val handler = Thread.currentThread().uncaughtExceptionHandler
    ?: Thread.getDefaultUncaughtExceptionHandler()
if (handler != null) {
    handler.uncaughtException(Thread.currentThread(), error)
} else {
    e("Constructor.io: Undeliverable unexpected exception (no uncaught handler): $error")
}

This is a reasonable approach, but note that on Android the default uncaught exception handler (set by the framework or crash reporters like Firebase Crashlytics) will terminate the process. The PR summary says "internal network errors should never bring down the consumer's application" — that goal holds for IOException and InterruptedException, but unexpected exceptions (e.g., NullPointerException, IllegalStateException) can still crash the app via this path. This is arguably the correct behavior (you don't want to silently swallow logic errors), but the PR description could be more precise about which exceptions are handled gracefully vs. which can still cause crashes.

[File: ConstructorIo.kt — setupRxJavaErrorHandler()] Consider logging unexpected exceptions before forwarding

When forwarding to the uncaught exception handler, there is no log statement:

if (handler != null) {
    handler.uncaughtException(Thread.currentThread(), error)
}

If the app does not crash (e.g., a custom uncaught handler suppresses it), there will be no Constructor.io log entry. Consider adding an e() call before forwarding to aid diagnostics:

if (handler != null) {
    e("Constructor.io: Forwarding undeliverable unexpected exception to uncaught handler: $error")
    handler.uncaughtException(Thread.currentThread(), error)
}

[File: ConstructorIo.kt — placement of setupRxJavaErrorHandler()] Minor: the call to setupRxJavaErrorHandler() is placed before this.context = context.applicationContext is actually needed by the handler, which is fine. But the comment says "Install the error handler early, before component initialization triggers the Dagger graph" — the handler itself has no dependency on the Dagger graph, so this ordering is safe and the comment is accurate.

[File: NetworkModule.kt] createWithScheduler(Schedulers.io()) — interaction with RxSchedulersOverrideRule in tests

The test rule RxSchedulersOverrideRule overrides the IO scheduler with Schedulers.trampoline():

RxJavaPlugins.setIoSchedulerHandler(schedulerFunction)

This means that in unit tests, RxJava2CallAdapterFactory.createWithScheduler(Schedulers.io()) will still use trampoline (because Schedulers.io() is resolved at subscription time through the plugin hook). This is correct behavior and the existing test suite should continue to work. No action needed, but worth noting for anyone reviewing the test compatibility.

[File: ConstructorIoRxErrorHandlerTest.kt] Tests are well-structured and comprehensive

The test coverage is good:

• Idempotency ✓
• Non-overwrite of existing handler ✓
• IOException handling ✓
• InterruptedException with interrupt flag restoration ✓
• Raw (unwrapped) IOException ✓
• SocketTimeoutException (subclass) ✓
• Forwarding to uncaught handler ✓
• Fallback logging when no handler exists ✓
• Null cause edge case ✓

One gap: there is no test verifying that a log message is emitted for IOException or InterruptedException. Since e() calls android.util.Log (not available in JVM unit tests without Robolectric or mocking), this is likely intentional and acceptable. A comment acknowledging this limitation would be helpful.

[File: ConstructorIoRxErrorHandlerTest.kt Line: L18] Unnecessary var for singleton reference

private var constructorIo = ConstructorIo

Since ConstructorIo is a Kotlin object (singleton), this field is a direct reference to the singleton and cannot be reassigned to a different instance. It should be val:

private val constructorIo = ConstructorIo

---

Conclusion

This is a well-motivated, cleanly implemented fix for a real production issue. The PR description is thorough and the test coverage is solid. The main items worth addressing before merge are:

1. (Minor, optional) Add a log line before forwarding unexpected exceptions to the uncaught handler, for easier diagnostics.
2. (Nit) Change var to val for the constructorIo field in the test file.
3. (Doc) Clarify in the PR/KDoc that unexpected exceptions (non-IO, non-Interrupt) can still crash the app via the uncaught exception handler — which is intentional but worth stating.

Overall the change is approvable. Good work addressing a subtle but impactful SDK stability issue.

[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]

@HHHindawy thoughts?

[esezen]

Looking great. I left some comments

[esezen]

Claude mentioned a race condition for this one:

setupRxJavaErrorHandler() has a small race condition. The getErrorHandler() != null check and the setErrorHandler() call are not atomic. If two threads call init() concurrently, or a host app sets its own handler between the check and the set, one handler could be silently overwritten.

Consider adding @synchronized:

@Synchronized
internal fun setupRxJavaErrorHandler() {
  if (RxJavaPlugins.getErrorHandler() != null) return
    RxJavaPlugins.setErrorHandler { throwable ->
      // ...
    }
}

Since ConstructorIo is a Kotlin object, @synchronized locks on the singleton instance, making the check then-set atomic with negligible performance cost (it only runs once during init).

[esezen]

@HHHindawy thoughts?