[CDX-358] Suppress undeliverable RxJava errors

[HHHindawy]

No description provided.

[constructor-claude-bedrock]

Code Review Summary

This PR introduces a global RxJava error handler to prevent app crashes from undeliverable exceptions (specifically network timeouts like SocketTimeoutException). The implementation adds an optional suppressRxErrors configuration flag that, when enabled, installs a global error handler to catch and log these exceptions instead of crashing the app. The feature is well-tested with comprehensive unit tests.

Detailed Feedback

Positive Aspects

1. Well-documented implementation: The code includes clear KDoc comments explaining the purpose and behavior of the error handler (ConstructorIo.kt:103-107, 112-115).

2. Excellent test coverage: The new test file ConstructorIoRxErrorHandlerTest.kt covers multiple scenarios including SocketTimeoutException, IOException, InterruptedException, and other exceptions.

3. Opt-in by default: The suppressRxErrors flag defaults to false, making this a non-breaking change that requires explicit opt-in.

4. Testable design: The createRxErrorHandler() function is marked internal for testing purposes, which is a good practice.

Issues and Concerns

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Line: L109] - CRITICAL: Potential RxJavaPlugins conflict

The code calls RxJavaPlugins.setErrorHandler() directly without checking if an error handler is already set. According to RxJava documentation, calling setErrorHandler() when a handler is already registered can cause issues. Consider:

private fun setupRxErrorHandler() {
    // Check if handler is already set to avoid conflicts
    if (RxJavaPlugins.getErrorHandler() == null) {
        RxJavaPlugins.setErrorHandler(createRxErrorHandler())
    }
}

However, there's a deeper architectural concern: RxJavaPlugins are global singletons. If your library sets a global error handler, it could conflict with:

• The host application's own RxJava error handler
• Other libraries that also set error handlers
• Test frameworks (as seen in RxSchedulersOverrideRule.kt:41 which calls RxJavaPlugins.reset())

Recommendation: Document this limitation clearly in the KDoc and consider alternative approaches like wrapping observables with proper error handling operators instead of global handlers.

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Line: L116-128] - Code quality: Error unwrapping logic

The error unwrapping logic throwable.cause ?: throwable is good, but the distinction between IOException/InterruptedException and other exceptions may not be meaningful since both are logged at error level (using e()). Consider:

internal fun createRxErrorHandler(): (Throwable) -> Unit = { throwable ->
    val error = throwable.cause ?: throwable
    
    when (error) {
        is IOException, is InterruptedException -> {
            // Network errors and interruptions are expected in normal operation
            e("Non-fatal network error: ${error.javaClass.simpleName}: ${error.message}")
        }
        else -> {
            // For unexpected errors, include stack trace for debugging
            e("Undeliverable exception: ${error.javaClass.simpleName}: ${error.message}")
            error.printStackTrace()
        }
    }
}

This improvement:

• Includes the exception class name for better debugging
• Adds printStackTrace() for unexpected errors to aid troubleshooting
• Makes the differentiation between expected and unexpected errors more meaningful

[File: library/src/main/java/io/constructor/core/ConstructorIoConfig.kt Line: L28] - Documentation: Missing usage guidance

The KDoc for suppressRxErrors explains what it does but not when developers should use it. Consider adding:

/**
 * @property suppressRxErrors When true, sets up a global RxJava error handler to catch 
 * undeliverable network exceptions (e.g., SocketTimeoutException) that would otherwise crash 
 * the app. Defaults to false. 
 * 
 * **Important**: This sets a global RxJava error handler which may conflict with error 
 * handlers set by the host application or other libraries. Only enable this if you are 
 * experiencing crashes from undeliverable RxJava exceptions.
 * 
 * **Recommended**: Instead of enabling this flag, consider handling errors properly in 
 * your RxJava subscription chains using operators like `onErrorReturn()` or `doOnError()`.
 */

[File: library/src/test/java/io/constructor/core/ConstructorIoRxErrorHandlerTest.kt Line: L27] - Test quality: Counting wrapper hides actual test

The test wraps the internal handler with a counting mechanism, but this means you're not actually testing the production error handler directly. Consider:

@Before
fun setup() {
    RxJavaPlugins.reset()
    errorCount = 0
    RxJavaPlugins.setErrorHandler(ConstructorIo.createRxErrorHandler())
}

// Add a RxJavaPlugins error handler wrapper that counts for assertions
private fun setupCountingHandler() {
    val productionHandler = ConstructorIo.createRxErrorHandler()
    RxJavaPlugins.setErrorHandler { throwable ->
        errorCount++
        productionHandler(throwable)
    }
}

Then use setupCountingHandler() in tests to ensure you're testing the actual production code path.

[File: library/src/test/java/io/constructor/core/ConstructorIoRxErrorHandlerTest.kt Line: L38-46] - Test coverage: Missing edge cases

Consider adding tests for:

1. Wrapped exceptions (e.g., UndeliverableException wrapping a SocketTimeoutException)
2. Null exception messages
3. Concurrent error handling
4. Behavior when the error handler itself throws an exception

[General] - Architecture: Consider alternative approach

The root cause of undeliverable exceptions is typically improper subscription management or missing error handling in Observable chains. Instead of a global error handler, consider:

1. Proper disposal: Ensure all subscriptions are properly disposed
2. Error operators: Use onErrorReturn(), onErrorResumeNext(), or doOnError() in Observable chains
3. Wrapper utilities: Create extension functions that add proper error handling:

fun <T> Observable<T>.withErrorHandling(): Observable<T> {
    return this.doOnError { error ->
        if (error is IOException || error is InterruptedException) {
            e("Network error: ${error.message}")
        }
    }.onErrorResumeNext(Observable.empty())
}

This approach is more localized, doesn't create global side effects, and gives developers more control.

[General] - Documentation: Missing integration guide

There's no documentation about when and why developers should enable suppressRxErrors. Consider adding a section to the README or creating a migration guide for developers experiencing these crashes.

Conclusion

The implementation is functionally correct and well-tested, but the use of a global RxJava error handler is architecturally concerning due to potential conflicts with the host application and other libraries.

Recommendations:

1. Document the global nature and potential conflicts clearly in the KDoc and README
2. Consider alternative approaches that don't rely on global state (error handling operators, wrapper utilities)
3. Add a check to avoid overwriting existing error handlers
4. Improve error logging to include more debugging information for unexpected errors
5. Add edge case tests for wrapped exceptions and null messages

If the team decides to proceed with this approach, I recommend making the improvements to error handling, documentation, and adding the conflict check before merging.

Overall assessment: Changes Requested - The code works as intended, but architectural concerns and missing safeguards should be addressed before merging.