Use Observable.create() to construct LocationServicesOkObservable

[thuytrinh]

Fix #404 again.

[CLAassistant]

All committers have signed the CLA.

[dariuszseweryn]

We may end up leaking a context if this will get called before the subscribe finishes

[thuytrinh]

Good catch @dariuszseweryn 👍 There may be a case like that. For example, considering the following execution flow:

• The program passes by line 51 at if (!emitter.isDisposed()) {. There's still at least a subscriber of this Observable, so the context.registerReceiver() is about to be executed.
• Then, somehow no subscriber subscribes to the Observable anymore. The code in setCancellable() gets invoked.
• Then context.registerReceiver() gets executed. Thus the leak has just happened.

I'm thinking about doing another check if (!emitter.isDisposed()) right after context.registerReceiver() but it looks kinda weird. I never wrote such conditional flow before. Wdyt?

[thuytrinh]

Or another idea is to move the setCancellable() into the isDispose() check. For example:

if (!emitter.isDisposed()) {
    context.registerReceiver(broadcastReceiver, new IntentFilter(LocationManager.MODE_CHANGED_ACTION));
    emitter.onNext(initialValue);
    emitter.setCancellable(new Cancellable() {
        @Override
        public void cancel() throws Exception {
            try {
                context.unregisterReceiver(broadcastReceiver);
            } catch (IllegalStateException ignored) {
            }
        }
    });
}

Wdyt?

[thuytrinh]

I've changed the impl into below based on @akarnokd's suggestion link:

    public Observable<Boolean> get() {
        return Observable.create(new ObservableOnSubscribe<Boolean>() {
            @Override
            public void subscribe(final ObservableEmitter<Boolean> emitter) throws Exception {
                final boolean initialValue = locationServicesStatus.isLocationProviderOk();
                final BroadcastReceiver broadcastReceiver = new BroadcastReceiver() {
                    @Override
                    public void onReceive(Context context, Intent intent) {
                        final boolean newValue = locationServicesStatus.isLocationProviderOk();
                        emitter.onNext(newValue);
                    }
                };
                emitter.onNext(initialValue);
                context.registerReceiver(broadcastReceiver, new IntentFilter(LocationManager.MODE_CHANGED_ACTION));
                emitter.setCancellable(new Cancellable() {
                    @Override
                    public void cancel() throws Exception {
                        try {
                            context.unregisterReceiver(broadcastReceiver);
                        } catch (IllegalStateException ignored) {
                        }
                    }
                });
            }
        }).distinctUntilChanged();
    }

I think this impl is able to prevent the above leak from happening.

[dariuszseweryn]

Yup, but we should get rid of the try/catch in the Cancellable as it could only hide potential issues

[thuytrinh]

@dariuszseweryn Good call 👍 Done.

[uKL]

Looks good to me :) #notasubclassingfananyway

[dariuszseweryn]

🚀

[dariuszseweryn]

Awesome! Thank you for the contribution.