AndroidNetworkListener.startListening() should not ignore TooManyRequestsException

[keithsmyth-thescore]

Expected Behavior

Surface the crash to allow the developer to debug closer to the source of the issue as Google intended.

Current Behavior

AndroidNetworkListener.startListening() currently swallows the exception.

    fun startListening() {
        try {
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
                setupNetworkCallback() // <- this calls registerNetworkCallback
            } else {
                setupBroadcastReceiver()
            }
        } catch (throwable: Throwable) {
            // We've seen issues where we see exceptions being thrown by connectivity manager
            // which crashes an app. Its safe to ignore these exceptions since we try our best
            // to mark a device as offline
            // Github Issues:
            // https://github.com/amplitude/Amplitude-Kotlin/issues/220
            // https://github.com/amplitude/Amplitude-Kotlin/issues/197
            logger.warn("Error starting network listener: ${throwable.message}")
        }
    }

The issues linked don't show understanding or research into the root cause. Whether its mishandling of Amplitude sdk or another sdk leaking the NetworkCallback calls.

Possible Solution / Reasoning

Rethrow the crash immediately, and perhaps link to and update documentation to help developers find the root cause. Point out tear down call when showing how to configure Amplitude sdk https://amplitude.com/docs/sdks/analytics/android/android-kotlin-sdk#configure-the-sdk

Perhaps encourage a singleton instance of the Amplitude class, and detail how it can leak ConnectivityManager$NetworkCallback and lead to TooManyRequestsException if multiple instances are created. That's more your expertise.

Reasoning:
https://developer.android.com/reference/android/net/ConnectivityManager#registerNetworkCallback(android.net.NetworkRequest,%20android.net.ConnectivityManager.NetworkCallback)
ConnectivityManager.requestNetworkCallback (and a few other entry points) has the following documentation:

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 requestNetwork as well as 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 unregisterNetworkCallback(ConnectivityManager. NetworkCallback).

Meaning if a library is leaking its NetworkCallback the 100 concurrent callbacks registered can be hit fairly quickly. By swallowing the exception it is making it more difficult for developers to debug and find the root cause.

For us we were creating a new instance of the Amplitude class for every track request and not tearing it down properly. This was only caught as I was looking for NetworkCallback in the memory dump and saw nearly a hundred instances of networkCallbackForHigherApiLevels in memory. Genuinely well done on the naming though, I wouldn't have caught it if it was named badly.

I've seen WorkManager cop a lot of blame for leaking due to libraries sometimes applying this bandaid fix for a bug that should be surfaced. I feel as Android devs we should be surfacing this exception immediately, and then offering help to developers to track down the root cause.

Thanks for your time!

Steps to Reproduce

1. Create ~100 instances of the Amplitude sdk without tearing down. (Misuse I agree, but hard to track and could be helped with easy documentation updates for future players)

Environment

• Unity Plugin Version: 1.19.4 amplitude = { group = "com.amplitude", name = "analytics-android", version.ref = "amplitude" }
• Device: reproduced Across Pixel and Samsung
• Device OS and Version: reproduced in at least 14 and 15