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.

Sign up for GitHub

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

Jump to bottom

Use Observable.create() to construct LocationServicesOkObservable #438

Merged
merged 3 commits into from
Jun 8, 2018
Merged

Use Observable.create() to construct LocationServicesOkObservable #438

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc6b52b
Use `Observable.create()` to construct LocationServicesOkObservable
Jun 7, 2018
cd522bc
Move setCancellable to the bottom
Jun 7, 2018
0267e6d
Remove try-catch block
Jun 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rxandroidble/src/main/java/com/polidea/rxandroidble2/ClientComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.polidea.rxandroidble2.internal.scan.ScanSetupBuilderImplApi23;
import com.polidea.rxandroidble2.internal.serialization.ClientOperationQueue;
import com.polidea.rxandroidble2.internal.serialization.ClientOperationQueueImpl;
import com.polidea.rxandroidble2.internal.util.LocationServicesOkObservableApi23;
import com.polidea.rxandroidble2.internal.util.LocationServicesOkObservableApi23Factory;
import com.polidea.rxandroidble2.internal.util.LocationServicesStatus;
import com.polidea.rxandroidble2.internal.util.LocationServicesStatusApi18;
import com.polidea.rxandroidble2.internal.util.LocationServicesStatusApi23;
Expand Down Expand Up @@ -163,11 +163,11 @@ LocationServicesStatus provideLocationServicesStatus(
@Named(NamedBooleanObservables.LOCATION_SERVICES_OK)
Observable<Boolean> provideLocationServicesOkObservable(
@Named(PlatformConstants.INT_DEVICE_SDK) int deviceSdk,
Provider<LocationServicesOkObservableApi23> locationServicesOkObservableApi23Provider
LocationServicesOkObservableApi23Factory locationServicesOkObservableApi23Factory
) {
return deviceSdk < Build.VERSION_CODES.M
? ObservableUtil.justOnNext(true) // there is no need for one before Marshmallow
: locationServicesOkObservableApi23Provider.get();
: locationServicesOkObservableApi23Factory.get();
}

@Provides
Expand Down
56 changes: 0 additions & 56 deletions .../main/java/com/polidea/rxandroidble2/internal/util/LocationServicesOkObservableApi23.java
View file
Open in desktop

This file was deleted.

58 changes: 58 additions & 0 deletions ...ava/com/polidea/rxandroidble2/internal/util/LocationServicesOkObservableApi23Factory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package com.polidea.rxandroidble2.internal.util;

import android.annotation.TargetApi;
import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.location.LocationManager;
import android.os.Build;

import bleshadow.javax.inject.Inject;
import io.reactivex.Observable;
import io.reactivex.ObservableEmitter;
import io.reactivex.ObservableOnSubscribe;
import io.reactivex.functions.Cancellable;

@TargetApi(Build.VERSION_CODES.KITKAT)
public class LocationServicesOkObservableApi23Factory {
private final Context context;
private final LocationServicesStatus locationServicesStatus;

@Inject
LocationServicesOkObservableApi23Factory(
final Context context,
final LocationServicesStatus locationServicesStatus) {
this.context = context;
this.locationServicesStatus = locationServicesStatus;
}

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.setCancellable(new Cancellable() {
@Override
public void cancel() throws Exception {
try {
context.unregisterReceiver(broadcastReceiver);
} catch (IllegalStateException ignored) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@thuytrinh thuytrinh Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@thuytrinh thuytrinh Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dariuszseweryn Good call 👍 Done.

}
}
});
if (!emitter.isDisposed()) {
context.registerReceiver(broadcastReceiver, new IntentFilter(LocationManager.MODE_CHANGED_ACTION));
emitter.onNext(initialValue);
}
}
}).distinctUntilChanged();
}
}
16 changes: 8 additions & 8 deletions ...ationServicesOkObservableApi23Test.groovy → ...rvicesOkObservableApi23FactoryTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import hkhc.electricspock.ElectricSpecification
import org.robolectric.annotation.Config

@Config(manifest = Config.NONE)
class LocationServicesOkObservableApi23Test extends ElectricSpecification {
class LocationServicesOkObservableApi23FactoryTest extends ElectricSpecification {
def contextMock = Mock Context
def mockLocationServicesStatus = Mock LocationServicesStatus
def objectUnderTest = new LocationServicesOkObservableApi23(contextMock, mockLocationServicesStatus)
def objectUnderTest = new LocationServicesOkObservableApi23Factory(contextMock, mockLocationServicesStatus)
BroadcastReceiver registeredReceiver

def setup() {
Expand All @@ -24,7 +24,7 @@ class LocationServicesOkObservableApi23Test extends ElectricSpecification {
mockLocationServicesStatus.isLocationProviderOk() >> true

when:
objectUnderTest.subscribe()
objectUnderTest.get().subscribe()

then:
1 * contextMock.registerReceiver(!null, {
Expand All @@ -37,7 +37,7 @@ class LocationServicesOkObservableApi23Test extends ElectricSpecification {
given:
mockLocationServicesStatus.isLocationProviderOk() >> true
shouldCaptureRegisteredReceiver()
def disposable = objectUnderTest.test()
def disposable = objectUnderTest.get().test()

when:
disposable.dispose()
Expand All @@ -46,12 +46,12 @@ class LocationServicesOkObservableApi23Test extends ElectricSpecification {
1 * contextMock.unregisterReceiver(registeredReceiver)
}

def "should register and unregister broadcast listeners in correct order if unsubscribed on the first emission"() {
def "should still register and unregister in correct order"() {
given:
mockLocationServicesStatus.isLocationProviderOk() >> isLocationProviderOkResult

when:
objectUnderTest.take(1).test()
objectUnderTest.get().take(1).test()

then:
1 * contextMock.registerReceiver(_, _)
Expand All @@ -70,7 +70,7 @@ class LocationServicesOkObservableApi23Test extends ElectricSpecification {
mockLocationServicesStatus.isLocationProviderOk() >>> [true, false, true]

when:
def testObserver = objectUnderTest.test()
def testObserver = objectUnderTest.get().test()

then:
testObserver.assertValue(true)
Expand All @@ -95,7 +95,7 @@ class LocationServicesOkObservableApi23Test extends ElectricSpecification {
mockLocationServicesStatus.isLocationProviderOk() >>> [false, false, true, true, false, false]

when:
def testObserver = objectUnderTest.test()
def testObserver = objectUnderTest.get().test()

then:
testObserver.assertValue(false)
Expand Down