Weak reference to the subscriber

[benoitdion]

EventBus should keep a weak reference to the subscriber so registration aren't leaking objects when unregister is never called.

[renaudcerrato]

+1

 .../src/de/greenrobot/event/Subscription.java      | 39 +++++++++++++++-------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/EventBus-2.2.0/EventBus/src/de/greenrobot/event/Subscription.java b/eventbus-2.2.0/EventBus/src/de/greenrobot/event/Subscription.java
--- a/EventBus-2.2.0/EventBus/src/de/greenrobot/event/Subscription.java
+++ b/EventBus-2.2.0/EventBus/src/de/greenrobot/event/Subscription.java
@@ -15,10 +15,13 @@
  */
 package de.greenrobot.event;

+import java.lang.ref.WeakReference;
+
 final class Subscription {
-    final Object subscriber;
+    final WeakReference<Object> subscriber;
     final SubscriberMethod subscriberMethod;
     final int priority;
+
     /**
      * Becomes false as soon as {@link EventBus#unregister(Object)} is called, which is checked by queued event delivery
      * {@link EventBus#invokeSubscriber(PendingPost)} to prevent race conditions.
@@ -26,25 +29,37 @@ final class Subscription {
     volatile boolean active;

     Subscription(Object subscriber, SubscriberMethod subscriberMethod, int priority) {
-        this.subscriber = subscriber;
+        this.subscriber = new WeakReference<Object>(subscriber);
         this.subscriberMethod = subscriberMethod;
-        this.priority = priority;
-        active = true;
+        this.priority = priority;                
+        active = true;        
     }

     @Override
     public boolean equals(Object other) {
-        if (other instanceof Subscription) {
-            Subscription otherSubscription = (Subscription) other;
-            return subscriber == otherSubscription.subscriber
-                    && subscriberMethod.equals(otherSubscription.subscriberMethod);
-        } else {
-            return false;
-        }
+       
+        if (!(other instanceof Subscription))
+           return false;
+        
+        Subscription otherSubscription = (Subscription) other;
+        Object mySubscriber = subscriber.get();                        
+        Object otherSubscriber = otherSubscription.subscriber.get();
+            
+        if(mySubscriber == null || otherSubscriber == null) 
+           return false;
+            
+        return mySubscriber == otherSubscriber
+               && subscriberMethod.equals(otherSubscription.subscriberMethod);        
     }

     @Override
     public int hashCode() {
-        return subscriber.hashCode() + subscriberMethod.methodString.hashCode();
+       int hashCode = subscriberMethod.methodString.hashCode();
+       Object mySubscriber = subscriber.get();
+       
+       if(mySubscriber != null)
+           hashCode+=mySubscriber.hashCode();
+       
+        return hashCode;
     }
 }
\ No newline at end of file

[ScottPierce]

+1

[ghost]

+1

[renaudcerrato]

This is the only thing that keep me using that library.

[PerfectCarl]

+1

[gelldur]

+1

[sbhhbs]

+1

[sailor1861]

+1 Very Good.

[greenrobot]

I think both ways should be possible. Weak should be optional, because it might be intentional that the subscriber is kept alive by EventBus.

The problem with weak references is that it might suggest unregister is unnecessary. It is not in order to guarantee a deterministic behavior. The clearance of weakly referenced objects are not deterministic.

[renaudcerrato]

Even if weak references might suggest unregister is unnecessary, it's the sole responsability of the developer. I'm definitely not comfortable to pass a strong reference of my short lived activities/views to a bus object held by a long lived service (or whatever it could be).

You should consider using weak references while writing a warning to logcat if someone missed a call to unregister (as it happens with BroadcastReceivers).

[benoitdion]

+1 @nono240.

Relying on EventBus to keep an object alive isn't a good practice. It means your business classes are relying on implementation details of the bus.

[greenrobot]

Don't get me wrong, I'm in favor of adding weak references. Now, let's discuss what's the best way to do so.

My first thought was to introduce another method registerWeak(...). But then there might be too many register methods in different flavors (combination of sticky, future extensions, etc.). So, next thought was a general register(subscriber, options) for sticky/weak,etc. But how to do options right? Flags as bit values conflict with current method with priority, and an int is not descriptive. Enums are more descriptive but cannot be combined efficiently into one value (EnumSet is inefficient). Next thought was using an enum externally and a flag internally with varargs or several register methods with parameters option1, option2, ...

What do you think?

@benoitdion You always rely on implementation details (e.g. semantics of java.lang.String) - I don't let that count as bad practice. :)

[electrolobzik]

Guys, what is the case, when your activity/fragment doesn't call unregister method? I don't understand. In my case I always call unregister from onPause/onStop and it is always triggered. Activity/fragment can't be normally destroyed, without calling this method. Please, explain why you need weak references here? Only because of good practice rules or you had any real issue in your projects?

[gelldur]

Java doesn't have destructor so when fragment/activity is on the stack it doesn't receive notifications but it still live. Sometimes fragment is only a component without any view.

[electrolobzik]

@gelldur as I know android certainly calls onPause and onStop in this cases. This is why I ask if there are real examples, where weak reference is necessary. I suppose, that if programmer doesn't call unregister method from onPause/onStop it is his fault. And using weak reference in the library is concealment of the problem.

[gelldur]

Of course i can unregister there but as i sad. When activity is in background it doesnt receive events. I use API calls for EventBus so when user exits activity (maybe to another activity) and then when he hit back, previous activity don't get response.

1. Activity1 -- called register in onResume/onStart
2. calling API for response
3. now user go to another activity
4. Activity1 still lives but it is not visible so i must unregister because of onPause/onStop
5. New Activity2
6. Server response and no one catch Event because Activity1 unregister
7. User hit back now Activity1 is active

Activity2 could be a simple dialog which force Activity1 to call onPause :(

[electrolobzik]

1. If there are pop-up dialogs, you should just use onStop instead of onPause and everything will work as expected.
2. If Activity1 is not on the screen (is stopped already) i think it is not correct to process any events in it while it is in background. Because there is no guarantee that it ever will be on screen again. If you need to handle event triggered while activity was in background when it will come to the screen again, you should use sticky events or just use other sort of communication between parts of your application (Static variables in Application object, SharedPreferences, Sqlite etc).

[sbhhbs]

When I use event bus I don't usually have an UI element like activity, instead I may have some data model sending event to another that want register to these event. So if it is weak reference, I'll be sure my data model will get garbage collected.

ÔÚ 2014Äê11ÔÂ9ÈÕ£¬20:04£¬Roman Chernyak notifications@github.com дµÀ£º

If there are pop-up dialogs, you should just use onStop instead of onPause and everything will work as expected. If Activity1 is not on the screen (is stopped already) i think it is not correct to process any events in it while it is in background. Because there is no guarantee that it ever will be on screen again. If you need to handle event triggered while activity was in background when it will come to the screen again, you should use sticky events or just use other sort of communication between parts of your application (Static variables in Application object, SharedPreferences, Sqlite etc). ¡ª Reply to this email directly or view it on GitHub.

[electrolobzik]

@sbhhbs please, tell some more about your case. Your data model must be stored in some other object with life cycle managed by some application component (otherwise it will be already garbage collected at the moment of sending event), isn't it? All application components should have some point of life cycle where you can put call to unregister method. Maybe my conclusion is wrong, but i don't see other variants.

[sbhhbs]

@electrolobzik Well, what you said makes since, there's always a way to unregister. But imagine I have a ListAdapter whose ViewHolders of each item registers an event. I can of course unregister in the onPause on the Fragment by finding the ListView and ListAdapter it owns then get the ListAdapter's ViewHolders and unregister their events. But this makes it unnecessarily complex. I need to find a application component that able to do a callback when it's about to be dealloc or pause then go all the way down to the object that I want to unregister.

[greenrobot]

@sbhhbs It can be dangerous in some cases to use weak references in a UI context. Some functionality does not work in states when things get cleaned up by the system. If you do messy stuff without considering Android's life cycle, might even crash your app. Again, there is no strict guarantee when a weak reference is broken.

Having said that, I still think weak references makes sense for uses cases without strict timing requirements.

[electrolobzik]

@sbhhbs I understand your point of view, but I agree with @greenrobot that it is often dangerous to leave such views connected to the bus, while activity/fragment holding it is already in background. It can produce very strange unpredictable behavior and also unpredictable crashes. I suppose that it is better to have a rule "everything MUST be unregistered from event bus" than to get unpredictable behavior of your code. It is just lesser of two evils.

@greenrobot could you please give an example of such case "without strict timing requirements"?

[greenrobot]

@electrolobzik For example some data or manager class, that keeps some internal state updated by events. My take on that topic is to leave it up to the dev if he wants weak refs. With a clear warning in bold letters.

[gavares]

We provide an API to our users that allow them to subscribe to events via EventBus or use the more traditional delegate/listener pattern. Behind the scenes, our listeners just subscribe the appropriate events on the EventBus and then forward those events to methods in the user's listener.

Unfortunately, this means that if the user is creating short-lived listeners, they are leaking memory. A weak ref would be very useful here. Happy to provide an example if it would make my use case more clear.

[ubuntudroid]

Same applies to all types of views registering to event busses (e.g. to display a user's online state which can change over time). Updating such views consistently in the whole app can become a really tedious job which becomes a lot easier with an event bus. However there is no easy way for a view to tell that it is not being in use any more (apart from onDetachedFromWindow() which also has some drawbacks). A simple weak reference would remove a lot of hassle from the process in such cases!

[hoombar]

+1. Do you have an estimated date this will work its way in?

[greenrobot]

No. Probably won't make it into release 3.0.

[virl]

Please implement this.

[aprofromindia]

I would argue - this is just best practice for a pub-sub architecture.

[guidedways]

+1

[guidedways]

Read this, it's recommended that weak references be used to avoid memory leaks:

https://en.wikipedia.org/wiki/Observer_pattern

This isn't an alien concept. It's expected.

[techyourchance]

Hi guys.

IMHO, this is a very desirable feature for anyone who attempts to implement a modular approach, Single Responsibility Principle and a general separation of concerns in their apps.

Use case: I'd like to have some ProductProvider class which is injected into Activities and allows for retrieval of ProductEntity objects. ProductProvider either retrieves products from SQLite cache, or, if the product is not cached yet, initiates background data fetch from server (which is carried out by SyncAdapter). In the later case (fetch from server), when background sync completes I need to notify ProductProvider of that event, and EventBus comes in very handy here (much easier than ContentObservers and allows for more "informative" notifications).

Current approach: EventBus is injected into ProductProvider, and ProductProvider registers itself when it initiates a background server sync. Since several syncs can be initiated in parallel, ProductProvider keeps track of all the initiated syncs. When all syncs initiated by specific ProductProvider completed, ProductProvider unregisters from EventBus.

Drawbacks of current approach: 1) ProductProvider need to store and manage records of all the syncs it initiated 2) If any "sync completed" event is not sent (e.g. due to some minor bug), we leaked ProductProvider and all the objects strongly referenced from it 3) Fetch of data from server can take some time, therefore, even if all events are delivered, it might happen that this notification is not relevant anymore. The performance of the app could be better if ProductProvider would be GC'ed earlier.

Alternative approach: Activity which uses ProductProvider could register/unregister it to/from EventBus in its lifecycle callbacks, but that exposes implementation details of ProductProvider and introduces some "usage convention" which can't be enforced in practice.

This thread is more than two years old, but it looks like this feature is still not supported. Now, I really like EventBus and would gladly deep dive into its code and implement support for WeakReferences myself, but I'd like to be sure that this work, if done, is a desirable feature that will be merged into EventBus in the next release (otherwise I'll have hard time updating to new releases, while maintaining this manual addition).

So guys, what do you think about this?

[nateridderman]

At a minimum, you should add the unregister step to your README. Most users of your library will want to do this.

[IlyaEremin]

Any changes here? I catch memory leak when fast add\remove fragment from activity (it can occur when user move back and forth between parts of the app). I register fragment in onResume and unregister in onPause, so WeakReference will help a lot.

[Armenm]

+1

[nicjansma]

This recently caused me issues, not knowing EventBus was holding a strong reference on my Android fragments and other UI classes, eventually leading to a lot of OOMs.

It does look like a note was recently added to the README though! https://github.com/greenrobot/EventBus/commit/8bfdfe8dd2840393b260c93df3bf2bcc664dfe9d

[guidedways]

Not trying to turn people away from EventBus, but for this reason and a lot more, I've personally moved to using RxJava. Creating a simple 'bus' (which replaces EventBus entirely) in RxJava is a lot simpler than it sounds (literally 10 lines of code or less), and what you get is more of the deterministic behaviour you'd expect from a bus. With RxJava you could still cleanly dispose your consumables and avoid leaks, whilst maintaining code that is easier to read. You also get the much needed thread safety since you can pick a particular thread to observe on, while posting / subscribing on a completely different thread.

[william-ferguson-au]

Can you provide an example of the 10 lines of codes required to replace EventBus using RxJava?

[guidedways]

Sure

Here's my RxBus.java class

public class RxBus {
  public static final RxBus INSTANCE = new RxBus();

  private final Subject<Object> _bus;

  private RxBus() {
    _bus = PublishSubject.create().toSerialized();
  }

  public void post(Object o) {
    _bus.onNext(o);
  }

  public io.reactivex.Observable<Object> asObservable() {
    return _bus.hide();
  }
}

You post the same way:

RxBus.INSTANCE.post(new EventSomethingHappened());

And you can re-use the existing onEvent methods by observing like so:

RxBus.INSTANCE.asObservable()
        .observeOn(AndroidSchedulers.mainThread())
        .subscribe(event -> {
          if (event instanceof EventSomethingHappened) {
            onEventSomethingHappened();
          } else if (event instanceof EventSomethingElse) {
            onEventSomethingElse((EventSomethingElse) event);
          }
        }));

Where RxJava shines is in the following example, where you want to process only the last event fired, and ignore the previous ones that occurred within a given window (perfect for UI events that fire too often, such as a user tapping on a refresh button 10 times):

RxBus.INSTANCE.asObservable()
        .debounce(600, TimeUnit.MILLISECONDS) // <--- ignore all events fired within 600ms and grab the last one
        .subscribe(o -> {
           // do whatever
        }, e -> {
          // Handle error here
        }));

EventBus of course was never meant to solve the problems that RxJava does out of the box, which is why it's easier, lightweight and a lot better to simply use RxJava for the job. RxJava is brilliant when it comes to solving common problems that stem out of the observer-pattern. The example of debounce above, for instance, is priceless and there's no such mechanism available in EventBus. Ignoring all but the last event would require a lot of state saving and needless amount boilerplate code that you could otherwise live without.

RxBus also solves the problem of 'flowables' by handling back-pressure strategies for you, i.e. observables that are emitting faster than can be consumed (for e.g. a user typing into a text field while the observing 'search logic' takes 2 seconds on each keystroke, thus unable to catch-up. You can apply different back pressure strategies to such cases with RxJava easily, something EventBus should ideally support, but does not).

[anhvt52]

I also met leaking memory problem when register/unregister in onAttachedToWindow /onDetachedFromWindow of custom view because custom view holds the current activity(mContext) so onDetachedFromWindow never called and the Activity will be leaked. WeakReference is the easy way to handle the leak.