CycledLeScanner spawning Threads

[Del-S]

Hi,

Just checked 2.15.4 and it works great, just a slight issue. It spawns Threads for repeated scans using CycledLeScanner. I think that the issue is located in onDestroy():

if (mScanHelper.getCycledScanner() == null) {
    mScanHelper.getCycledScanner().stop();
    mScanHelper.getCycledScanner().destroy();
}

Seems to me that the condition should be != null.

[davidgyoung]

Thanks @Del-S. Clearly that is a typo in BeaconService.java. The correct logic is in an equivalent line in ScanJob.java -- I'm surprised nobody saw the consequence in testing until now.

My understanding of the consequence is that each time you call beaconManager.unbind(...) when service scanning is active, it will not properly stop the CycledScanner and it may leave scanning on.

I don't think that it should cause a thread leak because of the subsequent line that terminates the threads.

    if (mScanHelper.getCycledScanner() == null) {
        mScanHelper.getCycledScanner().stop();
        mScanHelper.getCycledScanner().destroy();
    }
    mScanHelper.getMonitoringStatus().stopStatusPreservation();
    mScanHelper.terminateThreads();

So while I agree that the == needs to change to a !=, I'd like to better understand the symptoms you are seeing so I know if this really is the cause.

[Del-S]

Ah I will try to describe it better. I'm running periodic scans using my own custom service that binds and unbinds beaconManager every time it runs. I was checking the scanner using Android profiler to figure out if the Threads are being terminated, most of them are, but after a while I noticed there are multiple instances of CycledLeScanner Thread there just sleeping.

[davidgyoung]

ok, how about if I prepare a test build with the == to != change, and you can run the same profiler test to see if it makes a difference?

[Del-S]

That would be awesome. I can check it as soon as tomorrow. Thank you.

[Del-S]

Oh and a question. Do you think that this issue can cause following Gatt error?

can't Register GATT client, MAX client reached: 32

[davidgyoung]

It may be possible. I would hope if properly implemented the Android Scanning APIs would recognize the same scan client starting a new scan can reuse existing scan clients. But it is possible it is not that smart

On Thu, Dec 20, 2018, 06:32 Del_S <notifications@github.com wrote:

Oh and a question. Do you think that this issue can cause following Gatt error?

can't Register GATT client, MAX client reached: 32

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/AltBeacon/android-beacon-library/issues/801#issuecomment-448966026, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_xcVF19mKZWSC1uVlKPtae-g_iBTRZks5u63VngaJpZM4ZPhlk .

[davidgyoung]

Change to fix this is implemented in #804

[davidgyoung]

@Del-S, please test with this release and see if it affects the issues you see:

https://github.com/AltBeacon/android-beacon-library/releases/tag/2.15.5-alpha1

[Del-S]

Thank you. I'm going to try it out tomorrow.

[Del-S]

@davidgyoung Seems to be fixed. Thread is killed after every scan. Thank you very much. :)