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

[GoogleCodeExporter]

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.

Original issue reported on code.google.com by alexrhel...@gmail.com on 5 Nov 2011 at 5:57

[GoogleCodeExporter]

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();
}

Original comment by alexrhel...@gmail.com on 5 Nov 2011 at 9:41

Attachments:

• main.java

[GoogleCodeExporter]

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.

Original comment by ogregoire on 7 Nov 2011 at 10:47

[GoogleCodeExporter]

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#getDe
claredAnnotations() :

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

Original comment by wasserman.louis on 15 Nov 2011 at 4:29

• Changed state: Accepted
• Added labels: Milestone-Release11

[GoogleCodeExporter]

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. 

Original comment by ogregoire on 16 Nov 2011 at 12:25

[GoogleCodeExporter]

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.

Original comment by wasserman.louis on 16 Nov 2011 at 2:44

[GoogleCodeExporter]

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

Original comment by ogregoire on 16 Nov 2011 at 4:02

[GoogleCodeExporter]

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.

Original comment by wasserman.louis on 16 Nov 2011 at 4:19

[GoogleCodeExporter]

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.

Original comment by ogregoire on 16 Nov 2011 at 4:51

[GoogleCodeExporter]

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").getA
nnotation(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.)

Original comment by stephan...@gmail.com on 16 Nov 2011 at 5:31

[GoogleCodeExporter]

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.

Original comment by ogregoire on 16 Nov 2011 at 5:39

[GoogleCodeExporter]

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.

Original comment by wasserman.louis on 16 Nov 2011 at 6:29

[GoogleCodeExporter]

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/

Original comment by wasserman.louis on 16 Nov 2011 at 6:59

[GoogleCodeExporter]

Issue 798 has been merged into this issue.

Original comment by wasserman.louis on 22 Nov 2011 at 6:20

[GoogleCodeExporter]

Original comment by yrfselrahc@gmail.com on 5 Dec 2011 at 6:35

• Removed labels: Milestone-Release11

[GoogleCodeExporter]

Original comment by fry@google.com on 10 Dec 2011 at 4:23

• Added labels: Package-EventBus

[GoogleCodeExporter]

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

Original comment by wasserman.louis on 17 Jan 2012 at 5:34

[GoogleCodeExporter]

Issue 879 has been merged into this issue.

Original comment by wasserman.louis on 20 Jan 2012 at 2:28

[GoogleCodeExporter]

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.

Original comment by kevinb@google.com on 31 Jan 2012 at 7:25

• Added labels: Milestone-Release12

[GoogleCodeExporter]

Rolling EventBus issues over to r13.

Original comment by cpov...@google.com on 23 Mar 2012 at 9:27

• Added labels: Milestone-Release13
• Removed labels: Milestone-Release12

[GoogleCodeExporter]

Fix checked in.

Original comment by wasserman.louis on 26 Apr 2012 at 2:44

• Changed state: Fixed

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:15

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:09

• Added labels: Restrict-AddIssueComment-Commit