-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[RFC] Improve support for auto-unsubscribing observables and newer language/API levels #12
Comments
I thinks there is no silver bullet here. For me the only rule is: always unsubscribe from observable when you don't need it anymore (onDestroy, onViewDestroy, etc.). In my case I just usually create set of CompositeSubsctiptions on activity/fragment events (onCreate, onResume) and unsubscribe from them in corresponding callbacks (onDestroy, onPause). about Activity life cycle callbacks: |
I usually do the same. The only thing that worries me using this approach is absence of guarantees that public static <T> Subscription safeSubscribe(final Observable<T> observable, final Observer<T> observer) {
final SafeObserver<T> delegate = new SafeObserver<T>(observer);
final Subscription subscription = observable.subscribe(delegate);
return new SafeSubscription(subscription, delegate);
}
private static class SafeSubscription implements Subscription {
private final Subscription subscription;
private final SafeObserver observer;
private SafeSubscription(final Subscription subscription, final SafeObserver observer) {
this.subscription = subscription;
this.observer = observer;
}
@Override
public void unsubscribe() {
subscription.unsubscribe();
observer.unsubscribe();
}
@Override
public boolean isUnsubscribed() {
return subscription.isUnsubscribed();
}
}
private static class SafeObserver<T> implements rx.Observer<T> {
private static final Observer<?> EMPTY = new EmptyObserver();
private volatile Observer<T> observer;
private SafeObserver(final Observer<T> observer) {
this.observer = observer;
}
public void unsubscribe() {
observer = (Observer<T>) EMPTY;
}
@Override
public void onCompleted() {
observer.onCompleted();
}
@Override
public void onError(Throwable e) {
observer.onError(e);
}
@Override
public void onNext(T t) {
observer.onNext(t);
}
} |
@mironov-nsk that's an interesting idea. Perhaps this can be combined with Since it's an |
I do something similar to @Yarikx Adding something similar to this class and building around it may be something useful to have for new library users.
The child class then implements Any subscription that is added after the fragment is created can be added using AFAIK this resolves the |
On a different note, I would not mind if support was only API 14+. Devices running a version below that only account for |
@austynmahoney I don't think we should add this (or similar) to library. I think it's better to write documentation in wiki about this 'common' pattern, and left implementation up to the user. |
Very true, without multiple inheritance we don't want to enforce anything. In a sample would be nice. |
@austynmahoney part of the problem is in #3 which the excerpt above does not cover. I like the sample idea where we demonstrate the "patterns" and best practices. |
I have recently been doing the below to handle this. public abstract class RxFragment extends Fragment {
PublishSubject<Void> detached = PublishSubject.create();
@Override
public void onDetach() {
super.onDetach();
detached.onNext(null);
}
public <T> Observable<T> bindObservable(Observable<T> in) {
return AndroidObservable.bindFragment(this, in).takeUntil(detached);
}
} The Take Until will cause onComplete to be called when onDetach is called, which will call unsubscribe (via SafeSubscriber). This means you just need to remember to wrap your observable in the bind method instead of adding it to a composite subscription. |
@DylanSale that's actually the neatest solution I've seen so far. I also like to idea of documenting these patterns somewhere. I was wondering, maybe code samples would work even better than a wiki? I'm personally not a huge fan of documentation that's too detached from code. There is already a samples module I started a while back, but it hasn't been integrated back into RxAndroid yet. The build setup is still giving me a headache, I might actually resort (again) to have subfolders inside the project that have their own Gradle wrappers, as the incompatibilities between the Android Gradle plugin and other plugins are getting in the way. Next week is really packed for me (I'll be in Stockholm from Wednesday throughout Sunday night), but I hope I can back to this the following week. |
@mttkay 👍 for building a cookbook! Not sure about pure source code samples. I think something like the OkHttp Recipes Page would be a lot more accessible. |
I usually avoid connecting Activity lifecycle with observables. We leverage loaders to get rid of activity lifecycle and the get an |
@DylanSale note that |
Very interesting thread ! I'm also scratching my head to find a fine way to handle Android objects' lifecycle in Rx. I had the idea to deal directly with the But I didn't find an operator which manipulates the public Observable bindObservable(Observable obs) {
return obs.onSubscribe( new OnSubscribe() { // This operator does not exists !
public void onSubscribe(Subscription sub) {
MyActivity.this.mCompositeSubscription.add( sub );
});
} Does this kind of operator exist ? |
@tbruyelle, I'm not sure if such operator exists, but it should be quite easy to implement. The following code should work: public <T> Observable<T> bind(final Observable<T> observable) {
return observable.lift(subscriber -> {
MyActivity.this.mCompositeSubscription.add(subscriber);
return subscriber;
});
} |
@tbruyelle see my code snippit above. It (or something like it using another lifecycle callback like onDestroy or onStop) avoids you having to deal with subscription objects at all. |
@roman-mazur I don't actually use onDetach, I just used that in my example in comparison to the previous ones. In reality I have a subject for each of the lifecycle callbacks in my fragment and activity base classes and bindObservable takes an enum selecting which one to take until. |
@mironov-nsk Thanks for the tip, I will give a try ! @DylanSale Yes I saw your code but fix me if I'm wrong it requires to use the |
@tbruyelle my solution requires SafeSubscriber which Observable.subscribe wraps all Subscribers in. It will work correctly as long as you use subscribe and not unsafeSubscribe. If you do use unsafeSubscribe then you can just call unsubscribe in your Subscriber's onComplete method. There isn't anything wrong with using CompositeSubscription, it is just a pain to always have to handle the Subscriptions. RxJava may not the best solution if you want to avoid wrappers. It uses them a lot. |
@DylanSale By wrappers I mean custom wrapper. I'm quite new in Rx development and I prefer to work directly with library objects for now, wrappers may add complexity to my learning. |
Subjects are just observables that are also Subscribers. They are used (sparingly) to connect imperative callback code (like the android life cycle callbacks) to Observables. My solution does not need any custom wrappers, SafeSubscriber is used internally (and automatically) by RxJava to make sure the Observable follows the Rx contract (no calls to oNext after onComplete etc). Using a custom Operator that has a side effect is not great RX style (as a rule things inside the Observable chain should not have side effects, this helps keep things thread safe). |
@DylanSale Oh ok, I didn't notice that |
AndroidObservable.bind makes sure the subscription is observed on the main thread and that the fragment or activity is not finishing, so is still somewhat necessary. |
@DylanSale Thanks for all the details, it's water clear now. |
From what I can gather the discussion here is centered around the create-destroy lifecycle. We should include resume-pause, start-stop as well as the fragment equivalents. For our purposes at Trello @dlew quickly solved this by separating out the state tracking. Rough outline is below:
LifecycleSubscription itself is fairly simple. Relevant parts below:
The other nice thing about a class like Making |
No you cannot use an annotation processor for modifying code and the |
I've finished a second draft based on In particular, look at Still no great solution to the If people like this, I'll work up some tests + javadocs for PR. |
I was not proposing to modify the code. Just to generate a super class. Yet On 02:33, Sun, Nov 23, 2014 Daniel Lew notifications@github.com wrote:
|
@JakeWharton what if for the a class such as:
The annotation processor generated something like:
With this approach we don't mess up with changing code and still allow for the "injection". The only downside would be that the manifest entry for the activity needs to be changed to the XXManaged class. |
Yeah AndroidAnnotations behaves like this, and it's.. well, kind of gross. My vote is to use the activity lifecycle callbacks and not re-invent the wheel. Pre-14 people can fend for themselves. |
Fair enough. I don't think there are enough advantages over the life cycle
|
Here's a simple illustration that compiles, done in 1 hour. If somebody is interested...
To see generated class. |
AOP approach would be simpler in implementation and usage though... |
Yeah but it's massively worse conceptually. Although the people who tolerate Retrolamba probably wouldn't care! |
Spent another 30 minutes :)
It might be not necessary to override methods.
@JakeWharton Do you think it reduces boilerplate or actually introduces it? :) |
One thing to note with using I tweaked the Subscriber to not have that distinction, so it wasn't ultimately a big deal, just something that didn't crop up before moving to using |
@JakeWharton Even if we used the activity lifecycle callbacks that would still not solve the problem for I agree that this version of annotation processing seems to smell. Anytime you're explicitly using classes that don't actually exist until you build worries me. |
@DylanSale That's a really good point. I can see a lot of devs being tripped up by it. For example, if I create an
|
My 2c after reading the entire discussion few times and have seen @dlew's PR:
I would love to see a simpler solution, and I quickly drafted something in a gist. It is basically a mix of some of the ideas above:
|
From your gist it looks like your solution still requires inheritance. More That solution looks a lot like my original work. The problem is that it When I really reduced the problem to its core parts I realized the On Sun, Nov 23, 2014, 15:06 Antonio Bertucci notifications@github.com
|
I love to have Observable. I almost did it when I worked on bindView(), and was thinking about the generalisation. I'm eager to rewrite bindView() using LifecycleObservable. |
@dlew I don't want to pollute this thread too much, so I put some comment on my gist. We might want to continue the discussion there. |
Not sure if their implementation is all that great, but the Facebook SDK uses a lifecycle helper class that is similar to the suggested implementation here. A lot of developers may already be used to having to call a helper in I'm not sure you can make it any nicer if you aren't subclassing |
We all really must accept a need to use AOP on Android. And here is how to do it right. What we haveThere are plenty of similar tools: AndroidAnnotations, ButterKnife, FragmentArgs, IcePick and now this "LifecycleManager", inserting a line of code in well-known locations to greatly improve code quality, turning Java code into a mix of powerful domain-specific languages. Those lines are small, but their count grows, showing tendency to turn arbitrary object lifecycle callbacks into small event buses, distributing events between multiple libraries. Those buses don't need commonly accepted event identifiers and notification strategies, instead they rely on common API, provided by class authors, and operate by means of bytecode itself. Every lifecycle method of any object amounts to an event. What we needImagine something like Mimic in reverse - like AspectJ advices, but without diverting into different language or going down to bytecode level: @PluginFor({Fragment.class})
public abstract class ButterknifeFragmentInjection extends Fragment {
@PlugInto(PluggingMode.AT_BEGINNING)
public void onViewCreated(View view) {
ButterKnife.inject(this, view);
}
} Simple, typesafe and straightforward. Extending Fragment here is just a convenience: the code can be written in Java, but rather than being turned into bytecode during compilation it's source code would be distributed with library to act as template for actual class, created by annotation processor: class ExampleFragment$$Helper {
private final WeakReference<ExampleFragment> instance;
ExampleFragment$$Helper(ExampleFragment instance) {
this.instance = new WeakReference(instance);
}
public void onViewCreated(View view) {
final ExampleFragment self = instance.get();
ButterKnife.inject(self, view);
...
self = null;
}
public void onSaveInstanceState(Bundle state) {
final ExampleFragment self = instance.get();
Icepick.saveInstanceState(self, state);
self = null;
}
...
} The only things, that would have to be inserted directly in bytecode of What should be donePretty much everything needed is already here: ready-to-go annotation processing and bytecode manipulation tools already exists. The only real challenge is avoiding obscure side effects and general lack of transparency, that may come from abusing AOP, but with source code being before developer's eyes that would not be as much of issue. PS Wow, it escalated into a plan of little NIH-induced AOP framework faster than I expected. Still, I believe, that this has at least indirect relation to main topic of this issue, so please comment on the idea, if you have anything to say. Similar solutions, ideas, perhaps? |
AspectJ already does both type-safe declaration in Java and weaving in the bytecode. Also, no. Just no. You'll never get everyone on board with this. |
RxAndroid-AspectJ integration, when?
I am sorry, if I have created an impression of someone seeking to steal your time here :) Would you, please, describe a solution to problem in question, using AspectJ? I am not a tool fanatic, so if that solution had even half of features I need, I would gladly use it. The concept, described at #93 is fine, but sadly useless in most cases, because it means giving up on inheritance altogether. |
I meant that AspectJ already allows you to write directly in Java and weave class files at compile-time rather than at runtime. An AOP-based approach is compelling if you have the tolerance for bytecode-manipulating tools. I just think it would be better served as a standalone project than something directly in a library like this one. |
Look at this code: public FooActivity extends Activity {
@InjectView(R.id.text) TextView text;
public void onCreate(Bundle savedInstanceState) {
setContentView(R.layout.text);
}
public void onPostCreate(Bundle savedInstanceState) {
text.setText("Is this good enough?");
}
} Let's say, that it works. Does it matter, if the code relies on
@JakeWharton, It still does too little to be trully useful. If I were to tell you, that I 'd like to see a better version of AspectJ without separate language elements (please, don't pretend, that those do not exist), with pointcuts and most of joint types removed and source code of aspects being properly viewable and debuggable, would that be descriptive enough? Just move those code blobs from magical inner classes into source files, generated by annotation processor, and it would be exactly, what I described above. If creating something like that were possible on top of AspectJ in it's current state.. |
@Alexander-- I think a lot of people (including me) would like to see/use annotations as a solution, but the aim here is to maintain this module as closer as possible to nuts and bolts. |
You can't generate inner-classes with an annotation processor. Also I've lost track of whatever you're getting at so unless there's a concrete proposal you're better off playing in a separate project. This project has a hard enough time figuring out what should actually be included without conflating other opinionated technologies. |
Do we consider #75 as good enough of a solution for now? |
As part of #172 the auto-unsubscribing things have been removed for future release. At this time, there's a wide variety of ways people create streams and want to unsubscribe from them. There's also a lot of potentially useful magic solutions that will appeal to a subset of users but not all. Knowledge of the strong references that are kept and continually trying to improve the pattern of using RxJava on Android will be the path forward for now unless some amazingly awesome solution that everyone can agree on and use. |
Currently, using Observables in Android components that have access to
Context
(or are themselves) requires one to carefully think about detaching from these sequences as part of the component life-cycle, since otherwise a closure the sequence holds a (strong) reference to might leak the current context until the time the sequence finishes and releases all subscribers.A good example are configuration changes, where Android first destroys, then recreates an Activity, but observables might still be holding on to a subscriber created within the scope of that Activity. Even with retained fragments that detach from their host activity, it's easy enough to leak the attached context indirectly by holding on to a view first created through that context (every view in Android has a strong reference to the Activity context it was first created in.)
This ticket aims to find solutions to improve this situation, potentially by leveraging newer Android API levels where available, and/or add first class support for newer Java language levels (Java 8/Retrolambda)
Some suggestions that have been made already follow.
Android 14+ APIs
One option would be to make use of Activity life cycle callbacks:
http://developer.android.com/reference/android/app/Application.ActivityLifecycleCallbacks.html
This might allows us to unsubscribe once Android is about to destroy an activity.
Java 8 lambdas and Retrolambda
Part of the problem is that every "lambda" in Java 7 or below is an anonymous inner class that will hold a strong reference to the outer class, even if it's a pure function that does not access state outside the lambda. Java 8 + Retrolambda could help here, since pure functions will be materialized as static methods that hold no reference back to their owner.
http://cr.openjdk.java.net/~briangoetz/lambda/lambda-translation.html
https://github.com/orfjackal/retrolambda
Weak subscribers
We've taken several stabs at this and always dismissed the solution. The idea was to use WeakReferences to bind subscribers and lambdas. The problem with his approach is that it doesn't work with lambdas, since the only incoming reference would be weak, and they are eligible for GC the instant they're created. There might still be other ways to achieve this, however, I'll leave it open for discussion.
The text was updated successfully, but these errors were encountered: