Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scanBleDevices causes memory leak #607

Closed
seljabali opened this issue Sep 5, 2019 · 18 comments
Closed

scanBleDevices causes memory leak #607

seljabali opened this issue Sep 5, 2019 · 18 comments

Comments

@seljabali
Copy link

Summary

Doing a basic scan of ble devices causes a memory leak despite ending the scan subscription at onStop().

Library version

1.10.1

Preconditions

Have sample app cloned & loaded: https://github.com/seljabali/rxandroidble2-leak

Steps to reproduce actual result


1. Enable Bluetooth on device

2. Enable GPS

3. Open app

4. Hit back button

5. Pull system drop down

6. Wait for Canary to analyze heap

7. Canary momentarily shows leak.

Actual result

  • Canary showing a memory leak

Expected result

  • Canary not showing a memory leak

Minimum code snippet reproducing the issue

scanDisposable = RxBle.get()
    .scanBleDevices(
        ScanSettings.Builder()
            .setScanMode(ScanSettings.SCAN_MODE_LOW_LATENCY)
            .setCallbackType(ScanSettings.CALLBACK_TYPE_ALL_MATCHES)
            .build()
    )
    .subscribeOn(Schedulers.io())
    .observeOn(AndroidSchedulers.mainThread())
    .doOnError {
        onScanError() // commenting this out stops leak
    }
    .doOnNext {
    }
    .subscribe()

Logs from the application running with settings:

https://github.com/seljabali/rxandroidble2-leak/blob/master/leakLog.txt

@dariuszseweryn
Copy link
Owner

What device/OS have you tested on?

@seljabali
Copy link
Author

What device/OS have you tested on?

Samsung Galaxy S8

@dariuszseweryn
Copy link
Owner

What OS version?

@seljabali
Copy link
Author

What OS version?

Oreo: 8.0, Api: 26

@dariuszseweryn
Copy link
Owner

Correct me if I am wrong but it seems that the root-cause of the leak is

2019-09-04 18:25:36.760 20056-20556/com.seljabali.rxandroidble2leak D/LeakCanary: ┬
2019-09-04 18:25:36.760 20056-20556/com.seljabali.rxandroidble2leak D/LeakCanary: ├─ android.bluetooth.le.BluetoothLeScanner$BleScanCallbackWrapper
2019-09-04 18:25:36.760 20056-20556/com.seljabali.rxandroidble2leak D/LeakCanary: │    Leaking: UNKNOWN
2019-09-04 18:25:36.760 20056-20556/com.seljabali.rxandroidble2leak D/LeakCanary: │    GC Root: Global variable in native code
2019-09-04 18:25:36.760 20056-20556/com.seljabali.rxandroidble2leak D/LeakCanary: │    ↓ BluetoothLeScanner$BleScanCallbackWrapper.mScanCallback

Which is a part of Android OS — not library's?

@dariuszseweryn
Copy link
Owner

I have looked through the classes that are referenced by the trace but all of those should be garbage collected normally with their Observable chains. I do not see anything leaking in either ScanOperationApi21 (which is held by the OS) nor in FIFORunnableEntry which is held because the operation is not collected.

@dariuszseweryn
Copy link
Owner

I have modified the example so it does not use RxAndroidBle at all and run the vanilla API. Changes looks like this:

    var scanCallback: ScanCallback? = null

    private fun startScan() {
        scanCallback = object : ScanCallback() {
            override fun onScanFailed(errorCode: Int) {
                super.onScanFailed(errorCode)
                Log.e("onScanFailed", errorCode.toString())
                onScanError()
            }

            override fun onScanResult(callbackType: Int, result: ScanResult?) {
                super.onScanResult(callbackType, result)
                Log.e("onScanResult", callbackType.toString())
            }

            override fun onBatchScanResults(results: MutableList<ScanResult>?) {
                super.onBatchScanResults(results)
                Log.e("onBatchScanResults", results.toString())
            }
        }
        BluetoothAdapter.getDefaultAdapter().bluetoothLeScanner.startScan(scanCallback!!)
    }

    private fun stopScan() {
        BluetoothAdapter.getDefaultAdapter().bluetoothLeScanner.stopScan(scanCallback!!)
        scanCallback = null
    }

LeakCanary result:

2019-09-12 17:58:11.660 14017-14262/com.seljabali.rxandroidble2leak D/LeakCanary: HeapAnalysisSuccess(heapDumpFile=/data/user/0/com.seljabali.rxandroidble2leak/files/leakcanary/2019-09-12_17-57-50_005.hprof, createdAtTimeMillis=1568303891655, analysisDurationMillis=19414, applicationLeaks=[ApplicationLeak(className=com.seljabali.rxandroidble2leak.ScanBleFragment, leakTrace=
    ┬
    ├─ android.bluetooth.le.BluetoothLeScanner$BleScanCallbackWrapper
    │    Leaking: UNKNOWN
    │    GC Root: Global variable in native code
    │    ↓ BluetoothLeScanner$BleScanCallbackWrapper.mScanCallback
    │                                                ~~~~~~~~~~~~~
    ├─ com.seljabali.rxandroidble2leak.ScanBleFragment$startScan$1
    │    Leaking: UNKNOWN
    │    Anonymous subclass of android.bluetooth.le.ScanCallback
    │    ↓ ScanBleFragment$startScan$1.this$0
    │                                  ~~~~~~
    ╰→ com.seljabali.rxandroidble2leak.ScanBleFragment
    ​     Leaking: YES (Fragment#mFragmentManager is null and ObjectWatcher was watching this)
    ​     key = dc9bf023-d4b3-41a6-b2ec-1a8bf3566e01
    ​     watchDurationMillis = 5152
    ​     retainedDurationMillis = 149
    , retainedHeapByteSize=1933)], libraryLeaks=[])

I think that this issue should be checked against the newest Android OS version and reported on Google Issue Tracker

@seljabali
Copy link
Author

Great investigative efforts @dariuszseweryn! Thank you. I'll create a Google Issue accordingly, and post it back here as a comment -- closing ticket.

@dariuszseweryn dariuszseweryn added bug Bug that is caused by the library invalid bug-android Bug that is caused by Android OS and removed bug Bug that is caused by the library invalid labels Sep 13, 2019
@seljabali
Copy link
Author

Hey @dariuszseweryn. I've updated the project to target Q/Api29. In testing on an S8 using Android 9, I found that the ScanCallBack doesn't leak however the Rx implementation does leak. For convenience, I've created side by side fragments between the two implementations on the github project to easily switch between the two in its MainActivity.

@seljabali seljabali reopened this Sep 24, 2019
@dariuszseweryn
Copy link
Owner

Hello @seljabali — I do not have an Android phone with me to test this out, could you attach leak traces?

@seljabali
Copy link
Author

here you go @dariuszseweryn:

D: HeapAnalysisSuccess(heapDumpFile=/data/user/0/com.seljabali.rxandroidble2leak/files/leakcanary/2019-09-25_11-55-20_674.hprof, createdAtTimeMillis=1569437738372, analysisDurationMillis=16360, applicationLeaks=[ApplicationLeak(className=com.seljabali.rxandroidble2leak.ScanRxBleFragment, leakTrace=
D: ┬
D: ├─ android.bluetooth.le.BluetoothLeScanner$BleScanCallbackWrapper
D: │    Leaking: UNKNOWN
D: │    GC Root: Global variable in native code
D: │    ↓ BluetoothLeScanner$BleScanCallbackWrapper.mScanCallback
D: │                                                ~~~~~~~~~~~~~
D: ├─ com.polidea.rxandroidble2.internal.operations.ScanOperationApi21$1
D: │    Leaking: UNKNOWN
D: │    Anonymous subclass of android.bluetooth.le.ScanCallback
D: │    ↓ ScanOperationApi21$1.val$emitter
D: │                           ~~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableCreate$CreateEmitter
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableCreate$CreateEmitter.observer
D: │                                     ~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeOnObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableSubscribeOn$SubscribeOnObserver.downstream
D: │                                                ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableUnsubscribeOn$UnsubscribeObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableUnsubscribeOn$UnsubscribeObserver.downstream
D: │                                                  ~~~~~~~~~~
D: ├─ com.polidea.rxandroidble2.internal.serialization.FIFORunnableEntry$1
D: │    Leaking: UNKNOWN
D: │    Anonymous class implementing io.reactivex.Observer
D: │    ↓ FIFORunnableEntry$1.this$0
D: │                          ~~~~~~
D: ├─ com.polidea.rxandroidble2.internal.serialization.FIFORunnableEntry
D: │    Leaking: UNKNOWN
D: │    ↓ FIFORunnableEntry.operationResultObserver
D: │                        ~~~~~~~~~~~~~~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableCreate$CreateEmitter
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableCreate$CreateEmitter.observer
D: │                                     ~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableUnsubscribeOn$UnsubscribeObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableUnsubscribeOn$UnsubscribeObserver.downstream
D: │                                                  ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableMap$MapObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableMap$MapObserver.downstream
D: │                                ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableDoOnEach$DoOnEachObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableDoOnEach$DoOnEachObserver.downstream
D: │                                          ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableFlatMap$InnerObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableFlatMap$InnerObserver.parent
D: │                                      ~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableFlatMap$MergeObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableFlatMap$MergeObserver.downstream
D: │                                      ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeOnObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableSubscribeOn$SubscribeOnObserver.downstream
D: │                                                ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableObserveOn$ObserveOnObserver.downstream
D: │                                            ~~~~~~~~~~
D: ├─ io.reactivex.internal.operators.observable.ObservableDoOnEach$DoOnEachObserver
D: │    Leaking: UNKNOWN
D: │    ↓ ObservableDoOnEach$DoOnEachObserver.onError
D: │                                          ~~~~~~~
D: ├─ com.seljabali.rxandroidble2leak.ScanRxBleFragment$startScan$1
D: │    Leaking: UNKNOWN
D: │    Anonymous class implementing io.reactivex.functions.Consumer
D: │    ↓ ScanRxBleFragment$startScan$1.this$0
D: │                                    ~~~~~~
D: ╰→ com.seljabali.rxandroidble2leak.ScanRxBleFragment
D: ​     Leaking: YES (Fragment#mFragmentManager is null and ObjectWatcher was watching this)
D: ​     key = 8c767c81-79da-4310-bc2c-5bed74441c44
D: ​     watchDurationMillis = 14993
D: ​     retainedDurationMillis = 9985
D: , retainedHeapByteSize=1629)], libraryLeaks=[])

@dariuszseweryn
Copy link
Owner

This looks exactly as before, starting the leak at Android's BleScanCallbackWrapper. I will try to run it once I will get my hands on an API 29 device.

@dariuszseweryn
Copy link
Owner

Update — I have tested on Android 10, API 29 Google Pixel device and the problem seems to be fixed without any changes to the library. Closing. Thank you for the report.

(...)
2019-11-04 14:02:09.774 12741-12741/com.seljabali.rxandroidble2leak I/RxBle#Client: ScanResult{bleDevice=RxBleDeviceImpl{MAC='XX:XX:XX:XX:XX:XX', name=null}, rssi=-74, timestampNanos=2181930517163897, callbackType=CALLBACK_TYPE_ALL_MATCHES, scanRecord=[...]}
2019-11-04 14:02:09.880 12741-12741/com.seljabali.rxandroidble2leak I/RxBle#Client: ScanResult{bleDevice=RxBleDeviceImpl{MAC='XX:XX:XX:XX:XX:XX', name=null}, rssi=-73, timestampNanos=2181930584983644, callbackType=CALLBACK_TYPE_ALL_MATCHES, scanRecord=[...]}
2019-11-04 14:02:09.889 12741-12784/com.seljabali.rxandroidble2leak I/RxBle#CancellableDisposable: Scan operation is requested to stop.
2019-11-04 14:02:09.891 12741-12784/com.seljabali.rxandroidble2leak D/BluetoothAdapter: isLeEnabled(): ON
2019-11-04 14:02:15.107 12741-12777/com.seljabali.rxandroidble2leak D/LeakCanary: Checking retained object because app became invisible
2019-11-04 14:02:15.109 12741-12777/com.seljabali.rxandroidble2leak D/LeakCanary: No retained objects

@LeMimit
Copy link

LeMimit commented Jan 2, 2020

Hello !

Sorry I think that this leak is not fixed. I got the same problem in my code.
It is may be possible to reopen this issue ?

I did some investigations with my own code linked to this leak.
I am using the version 1.10.5 of the library and Android 10 on a OnePlus 6.

I got the following leak trace : https://gist.github.com/LeMimit/f532873e8c891ed4d89d874276c9441f
This is basically the same as the previous comment. Starting with BleScanCallbackWrapper.

I cloned the code at https://github.com/seljabali/rxandroidble2-leak. With ScanCallBackBleFragment using the vanilla API, I have indeed no leak, but I got one using ScanRxBleFragment which is using library API.

May be this is linked to the fact that in the library in ScanOperation:

final SCAN_CALLBACK_TYPE scanCallback = createScanCallback(emitter);

The emitter is used in the callback which keep a reference. The same emitter is subscribed in the fragment which keep a reference to the Fragment itself. When the fragment is destroyed we have a leak.

Thanks !

@seljabali
Copy link
Author

Pinging @dariuszseweryn, can case he didn't notified.

@dariuszseweryn
Copy link
Owner

dariuszseweryn commented Jan 16, 2020

There is one more thing to this issue that may be important as it may cause race conditions. Scan cancellation is asynchronous (all BLE actions are to be precise). So when the user disposes the stream actual scan stop happens on a different thread. Leak Canary can create a snapshot before the scan stop happens. This would also explain why I did not notice any leaks when I have tested on Android 10.
I do not have time at the moment but someone could verify. @seljabali ? @LeMimit ?

Check could be as simple as:

  1. start scan
  2. stop scan
  3. wait 1 second
  4. put app to background

@dariuszseweryn dariuszseweryn added awaiting feedback and removed bug-android Bug that is caused by Android OS labels Mar 14, 2020
@seotrader
Copy link
Contributor

seotrader commented Jul 30, 2020

Hello everyone,

Is there any updates about this leak? I do have the same leak happening on Android 9 using RX

Thanks

@seotrader
Copy link
Contributor

Hello,
I've created a PR with a solution to the issue

#708

Hope it can be merged soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants