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

+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.