Skip to content

Comments

Fix network callback and lifecycle observer not being unregistered during disconnect#6125

Merged
VelikovPetar merged 6 commits intodevelopfrom
bug/AND-1040_fix_network_monitor_not_cleaned_up
Feb 4, 2026
Merged

Fix network callback and lifecycle observer not being unregistered during disconnect#6125
VelikovPetar merged 6 commits intodevelopfrom
bug/AND-1040_fix_network_monitor_not_cleaned_up

Conversation

@VelikovPetar
Copy link
Contributor

@VelikovPetar VelikovPetar commented Feb 3, 2026

Goal

During ChatClient.disconnect(), the disposeObservers() call was launched in a fire-and-forget coroutine that could be cancelled by userScope.cancelChildren() before completing. This left network callbacks and lifecycle observers registered.

Note: This is a potential cause for TooManyRequestsException thrown by android.net.ConnectivityManager.registerNetworkCallback.

// ConnectivityManager.java

/**
     * Registers to receive notifications about all networks which satisfy the given
     * {@link NetworkRequest}.  The callbacks will continue to be called until
     * either the application exits or {@link #unregisterNetworkCallback(NetworkCallback)} is
     * called.
     *
     * <p>To avoid performance issues due to apps leaking callbacks, the system will limit the
     * number of outstanding requests to 100 per app (identified by their UID), shared with
     * all variants of this method, of {@link #requestNetwork} as well as
     * {@link ConnectivityDiagnosticsManager#registerConnectivityDiagnosticsCallback}.
     * Requesting a network with this method will count toward this limit. If this limit is
     * exceeded, an exception will be thrown. To avoid hitting this issue and to conserve resources,
     * make sure to unregister the callbacks with
     * {@link #unregisterNetworkCallback(NetworkCallback)}.
     *
     * @param request {@link NetworkRequest} describing this request.
     * @param networkCallback The {@link NetworkCallback} that the system will call as suitable
     *                        networks change state.
     *                        The callback is invoked on the default internal Handler.
     * @throws RuntimeException if the app already has too many callbacks registered.
     */
    @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE)
    public void registerNetworkCallback(@NonNull NetworkRequest request,
            @NonNull NetworkCallback networkCallback) {
        registerNetworkCallback(request, networkCallback, getDefaultHandler());
    }

resolves: https://linear.app/stream/issue/AND-1043/crash-in-chateventsobservable

Implementation

ChatSocket.kt:

  • Call disposeNetworkStateObserver() synchronously (not in a coroutine) so it cannot be cancelled
  • Only the lifecycle observer disposal remains in a launched coroutine
  • Remove unused disposeObservers() function

StreamLifecycleObserver.kt:

  • Use NonCancellable context in dispose() to ensure lifecycle.removeObserver() completes even if the parent coroutine is cancelled

🎨 UI Changes

No UI changes.

Testing

This can be reproduced by rapidly logging in and logging out. In about 1 out of 10 logouts, ChatSocket.userScope is cancelled before the clean-up completes. After this change, it should never fail to unregister the network and lifecycle observers.

Summary by CodeRabbit

Bug Fixes

  • Improved cleanup of network callbacks and lifecycle observers during disconnection to ensure proper resource unregistration.

@VelikovPetar VelikovPetar force-pushed the bug/AND-1040_fix_network_monitor_not_cleaned_up branch from a1e06c3 to e673393 Compare February 3, 2026 12:35
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2026

SDK Size Comparison 📏

SDK Before After Difference Status
stream-chat-android-client 5.26 MB 5.26 MB 0.00 MB 🟢
stream-chat-android-offline 5.48 MB 5.48 MB 0.00 MB 🟢
stream-chat-android-ui-components 10.63 MB 10.63 MB 0.00 MB 🟢
stream-chat-android-compose 12.84 MB 12.85 MB 0.00 MB 🟢

@VelikovPetar VelikovPetar marked this pull request as ready for review February 3, 2026 14:34
@VelikovPetar VelikovPetar requested a review from a team as a code owner February 3, 2026 14:34
@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

Walkthrough

This PR fixes network callback and lifecycle observer cleanup during disconnection. It refactors handler management in StreamLifecycleObserver to ensure proper thread safety and context-aware cancellation, and modifies ChatSocket to execute disposeObservers synchronously with internal async cleanup of the lifecycle observer.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Added entry documenting the fix for network callback and lifecycle observer unregistration during disconnect.
Lifecycle Observer Refactoring
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/StreamLifecycleObserver.kt
Updated handler management flow: observe() now enqueues handlers within Main context and conditionally starts observing; dispose() uses NonCancellable + Main context to remove handlers and clean up observers. Added NonCancellable import.
Socket Disposal
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/socket/ChatSocket.kt
Changed disposeObservers() from suspend function to regular function. Network state observer is disposed directly; lifecycle observer disposal is deferred asynchronously via userScope coroutine launch.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • gpunto
  • andremion

Poem

🐰 Whiskers twitching with delight,
Cleanup flows both clean and tight,
Observers freed on disconnect's call,
Cancellation handled—no leaks at all!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: network callback and lifecycle observer unregistration during disconnect, which aligns with all the changes across ChatSocket.kt, StreamLifecycleObserver.kt, and CHANGELOG.md.
Description check ✅ Passed PR description comprehensively covers goal, implementation details, testing approach, and acknowledges no UI changes per template.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bug/AND-1040_fix_network_monitor_not_cleaned_up

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

PR checklist ✅

All required conditions are satisfied:

  • Title length is OK (or ignored by label).
  • At least one pr: label exists.
  • Sections ### Goal, ### Implementation, and ### Testing are filled.

🎉 Great job! This PR is ready for review.

@VelikovPetar VelikovPetar added the pr:bug Bug fix label Feb 4, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@VelikovPetar VelikovPetar merged commit e059665 into develop Feb 4, 2026
19 of 24 checks passed
@VelikovPetar VelikovPetar deleted the bug/AND-1040_fix_network_monitor_not_cleaned_up branch February 4, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants