don / cordova-plugin-ble-central

Bluetooth Low Energy (BLE) Central plugin for Apache Cordova (aka PhoneGap)
Apache License 2.0
947 stars 608 forks source link

scan resource leak #744

Open jseter opened 4 years ago

jseter commented 4 years ago

This is likely also the case in iOS and others; however, in the Android code - The CallbackContext used by scanning is only closed (keepalive=false) when there is an error. It is left open while scanning and its reference is replaced when a new scan is started. It is necessary to send a final message with keepalive=false (the default state of a plugin result) in order for the callback(s) to be removed from the JavaScript side. Each scan will create a new callback id, holding the success & failure functions with a strong reference. When done with the callback you can either send a silent message to remove the callback:

callbackContext.sendPluginResult(new PluginResult(PluginResult.Status.NO_RESULT));

or wire in an additional hook for observables, where one of the callbacks (success or failure) is multiplexed to provide an additional callback for complete. i.e.

scan: function (services, seconds, success, failure, complete) {
        var successWrapper = function(peripheral) {
            convertToNativeJS(peripheral);
            success(peripheral);
        };
        if(complete === undefined) {
            complete = function(){};
        }
        var failureWrapper = function(reason, type) {
            if(type === "NoError") {
                complete();
                return;
            }
            failure(reason);
        };
        cordova.exec(successWrapper, failureWrapper, 'BLE', 'scan', [services, seconds]);
    }

The dual parameter on failure is a consideration that code that bypasses the JavaScript wrapper will treat complete as an error, which is at least partially comparable to a final completion, and only bothers looking at the first argument. To accomplish multiple parameters on a callback:

    private static void error(CallbackContext target, String message, String type) {
        final List<PluginResult> messageList = new ArrayList<>();
        // status and keep alive are ignored on sub-parts of a multi-part message.
        messageList.add(new PluginResult(PluginResult.Status.NO_RESULT, message));
        messageList.add(new PluginResult(PluginResult.Status.NO_RESULT, type));
        target.sendPluginResult(new PluginResult(PluginResult.Status.ERROR, messageList));
    }
    private static void complete(CallbackContext target) {
        error(target, null, "NoError");
    }

Additionally the code at the end of findLowEnergyDevices does nothing:

        PluginResult result = new PluginResult(PluginResult.Status.NO_RESULT);
        result.setKeepCallback(true);
        callbackContext.sendPluginResult(result);

The JavaScript cordova handler will note that this pairing has no side-effects and just do nothing with it. It is unnecessary to include this.

Sadly however, ionic-native is only written to wire the next and error methods of an observer so a custom wrapper would have to be used to fully use an observable.

peitschie commented 2 years ago

For my own records, the behaviour here was changed in 443c9d1cfb7cedfb7a2341dedc68ea5c4b10d764, in response to #32

It looks like the bug has long since been fixed in cordova itself, so it should be safe to readd these clean-up steps again at some point. I'm very happy to take patches addressing this.