eventbus dispatches single event multiple times to most-derived listener if parents are listeners for same event

[gissuebot]

Original issue created by alexrhelder on 2011-11-05 at 05:57 AM

---

Consider the following:

public class main {

static class Foo {}
static class Bar extends Foo {}

static class BaseListener {
    @Subscribe
    public void foo(Foo foo) {
        System.out.println("base " + foo.toString());
    }
}

static class DerivedListener extends BaseListener {
    @Subscribe
    public void foo(Foo foo) {
        System.out.println("derived " + foo.toString());
    }
}

public static void main(String[] args) {

    EventBus eventbus = new EventBus("eventbus");

    DerivedListener d = new DerivedListener();
    eventbus.register(d);
    eventbus.post(new Bar());
}

}

The output will be:

derived foo main$Bar@99b5393
derived foo main$Bar@99b5393

I would expect derived foo to be called once.

[gissuebot]

Original comment posted by alexrhelder on 2011-11-05 at 09:41 PM

---

Included sample file which shows bug.

Looking at the sources, I think the problem is in AnnotatedHandlerFinder.

The method findAllHandlers() iterates over the the set containing the class of the 'listener' as well as classes of all its parents.

EventBus.handlersByType will (for the example above) have a single key (for Foo) but two entries for that key, because EventHandler[BaseListener.foo()] != EventHandler[DerivedListsner.foo()] -- the equals method returns false. So these are not collapsed into a single entry.

Class<?>.getMethods() already returns all of the public methods from this class as well as the inherited ones. Iterating over the superclasses of the listener does not make much sense to me...

I decided to port parts of guava eventbus to Android (it needed substantial modifications for caching reflection results and threading) and fixed the issue in my code by removing the outermost construct:

Class lazz = listener.getClass()
while (clazz != null) {
    ....
    clazz = clazz.getSuperClass();
}

[gissuebot]

Original comment posted by ogregoire on 2011-11-07 at 10:47 AM

---

I agree with you Alex: the @Subscribe interface specifies that it'll look for public methods, so in one way it makes sense to use Class.getMethods() to discover the annotated methods.

However, if I proxy my class (with Guice for instance or with any other system), I'd like EventBus to be able to still have it listening to the objects. Some proxy providers don't subclass the classes properly and the annotations become missing. Therefore it's nearly mandatory to have to use Class.getDeclaredMethods() of each parent class to find out which methods are actually annotated.

Therefore I think it might be needed to use only one key for a @Subscribe-annotated method and all its overrides in subclasses.

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-15 at 04:29 PM

---

I concur with your assessments.

ogregoire, as I understand it, Method.getAnnotation(Class<? extends Method>) includes annotations specified on the method in the superclasses. See the documentation of http://download.oracle.com/javase/6/docs/api/java/lang/reflect/Method.html#getDeclaredAnnotations() :

"Returns all annotations that are directly present on this element. Unlike the other methods in this interface, this method ignores inherited annotations..."

implying that getAnnotation does not ignore inherited annotations.

The other issue, which I didn't realize without some research, is that the @Subscribe annotation must be marked as @Inherited to get inherited by sub-methods.

I am amending this behavior, adding a couple test cases, and sending in a patch for review.

---

Status: Accepted
Owner: wasserman.louis
Labels: Milestone-Release11

[gissuebot]

Original comment posted by ogregoire on 2011-11-16 at 12:25 AM

---

Louis,

I'm not a native English speaker, so I might be completely wrong, but I understood the documentation as the exact opposite of what you wrote: "returns all annotations that are directly present on this element" means to me that all annotations as they are written on the method, not those written on overloaded elements present in the superclasses (this is implied by "directly"). To make sure, I'd run some tests as soon as I have a little more time.

Also, if I understand correctly the @Inherited annotation, it can be used only on an annotation @X (for instance) and that annotation @X will be transmitted to descendant of an element only if that element is a class.

http://download.oracle.com/javase/6/docs/api/java/lang/annotation/Inherited.html

This is what I understand from "Note that this meta-annotation type has no effect if the annotated type is used to annotate anything other than a class." as written in the docs.

This would mean that @Inherited on @Subscribe will be accepted by the compiler but will have no effect if the @Subscribe method is itself put on a method or another non-class member, but it will have an effect only on a class. Am I wrong?

I still think the only way is to iterate through all the methods of the class and its superclasses and register them if they are annotated, unless another method with the same signature is already registered. The biggest problem to this is to properly identify a method without its class.

This (naive) code extracts the proper signature of a method which can then be used to :

  private static String extractMethodSignature(Method method) {
    String s = method.toString();
    int parenthesisPos = s.indexOf('(');
    int signatureStart = s.lastIndexOf('.', parenthesisPos) + 1;
    return new String(s.substring(signatureStart)); // No need to keep the full signature in memory.
  }

I hope this can be fixed as I encountered the same case recently.

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-16 at 02:44 AM

---

No, you're right about that first part -- I was pointing out that a different method said that "unlike all the other methods" (the other methods including getAnnotation) "this method only looks at the annotations directly on this method."

I have a patch and it passes the most thorough test I could think of. Would you look at the test and see if it's convincing? See http://codereview.appspot.com/5389041/diff/1/guava-tests/test/com/google/common/eventbus/SubclassHandlerTest.java

In particular, I test the following cases:

• a method is marked as @Subscribe in a superclass but not overridden by a subclass
• a method is marked as @Subscribe in a superclass, and overridden in a subclass, which also annotates it with @Subscribe
• a method is marked as @Subscribe in a superclass, and overridden in a subclass, which does not annotate it with @Subscribe
• a method is not marked as @Subscribe in a superclass, but it is overridden in a subclass, which annotates it with @Subscribe
• a method is not present in a superclass, but is introduced by a subclass, which annotates it with @Subscribe

I tested that each of these methods was called exactly once, which is, I think, the expected behavior.

[gissuebot]

Original comment posted by ogregoire on 2011-11-16 at 04:02 PM

---

Thanks, this test case covers exactly the behavior that we expect and that feels natural.

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-16 at 04:19 PM

---

Yep. I'm really not clear on some of the documentation issues you brought up -- specifically, why @Inherited affects method annotations -- but I can't argue with the tests.

[gissuebot]

Original comment posted by ogregoire on 2011-11-16 at 04:51 PM

---

Well, I obviously don't understand @Inherited properly, since I expected the following test case to succeed:

http://pastebin.com/xJxDNUYr

My point was that, after reading the documentation, an @Inherited-annotation on a method was not propagated to the method that overrides the original one. The test case proved me wrong on line 46.

[gissuebot]

Original comment posted by stephan202 on 2011-11-16 at 05:31 PM

---

W.r.t. comment 8: are you sure you didn't make a copy/paste error? Lines 44 and 46 are self-contradicting. I assume line 46 was supposed to read:

assertNull(ChildClassWithInheritedAnnotation.class.getMethod("doSomething").getAnnotation(MyInheritedAnnotation.class));

That line does pass (as I guess you expected).

(N.B.: line 51 is identical to 49; I assume that's a similar c/p error.)

[gissuebot]

Original comment posted by ogregoire on 2011-11-16 at 05:39 PM

---

Indeed, you are right. I'm updating the class right now. I'm really sorry: I'm quite exhausted after all these Devoxx conferences.

So in fact, it works as I originally thought it would. And therefore I don't think that using @Inherited is appropriate in any solution for this issue.

But I understand you already found a proper fix.

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-16 at 06:29 PM

---

FYI, I removed the @Inherited annotation from @Subscribe, and the test fails. I'm really not sure if the JDK documentation is wrong, or what, but the evidence seems to support that conclusion.

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-16 at 06:59 PM

---

Correction, I am a n00b. My tests...started failing, with or without the @Inherited annotation? I fixed them now, though, and it doesn't need @Inherited. http://codereview.appspot.com/5389041/

[gissuebot]

Original comment posted by wasserman.louis on 2011-11-22 at 06:20 AM

---

Issue #798 has been merged into this issue.

[gissuebot]

Original comment posted by yrfselrahc on 2011-12-05 at 06:35 PM

---

(No comment entered for this change.)

---

Labels: -Milestone-Release11

[gissuebot]

Original comment posted by fry@google.com on 2011-12-10 at 04:23 PM

---

(No comment entered for this change.)

---

Labels: Package-EventBus

[gissuebot]

Original comment posted by wasserman.louis on 2012-01-17 at 05:34 PM

---

Bump bump; it's a fix to a pretty significant issue that needs review.

[gissuebot]

Original comment posted by wasserman.louis on 2012-01-20 at 02:28 PM

---

Issue #879 has been merged into this issue.

[gissuebot]

Original comment posted by kevinb@google.com on 2012-01-31 at 07:25 PM

---

Agreed that current behavior violates POLS.
Cliff is looking at this soon. It may not be easy to precisely nail down the right behavior, but we'll see.

---

Labels: Milestone-Release12

[gissuebot]

Original comment posted by cpovirk@google.com on 2012-03-23 at 09:27 PM

---

Rolling EventBus issues over to r13.

---

Labels: -Milestone-Release12, Milestone-Release13

[gissuebot]

Original comment posted by wasserman.louis on 2012-04-26 at 02:44 PM

---

Fix checked in.

---

Status: Fixed