EventBus single-threaded callback contract is violated under J9/1.7

[GoogleCodeExporter]

This is not directly a guava issue, rather a JVM bug (J9 bug) but perhaps it'll 
save somebody some research time/ headache.

J9 1.7 seems to be optimizing away the following snippet of code:

@Override public synchronized void handleEvent(Object event) throws InvocationTargetException { super.handleEvent(event); }

this unfortunately means that callback handlers are no longer under a monitor 
and multiple threads can enter them at will. This is fully reproducible and is, 
ehm, a problem if somebody is counting on single-thread-per-handler behavior.

Full reproduction code and information is at:
https://issues.apache.org/jira/browse/LUCENE-4987

Original issue reported on code.google.com by dawid.weiss@gmail.com on 8 May 2013 at 6:25

[GoogleCodeExporter]

Oh, a quick fix is to make the monitor explicit (move from method flag to 
synchronized (this) { super.handleEvent(event); }.

This isn't going to fix the JVM but will make guava users (on IBM JVMs) happier.

Original comment by dawid.weiss@gmail.com on 8 May 2013 at 8:27

[GoogleCodeExporter]

Sounds like a very low-cost fix. (Explicit locks are kinda just better anyway.) 
Any chance you can spot if we have this problem anywhere else?

Original comment by kevinb@google.com on 9 May 2013 at 8:25

• Changed state: Accepted

[GoogleCodeExporter]

We're waiting for a confirmation from J9 folks. Once we hear from them I'll 
know if this indeed is the right workaround or if it just happens to work for 
this particular test case/ code. I tried to reproduce this issue on a smaller 
example but couldn't so it's probably a particular mix of things that causes 
this frame to be eliminated. 

Once I know what this is I'll review the code and try to spot other places 
where this is the case (this should also be possible by applying an aspectj 
aspect, we will see).

Original comment by dawid.weiss@gmail.com on 9 May 2013 at 8:46

[GoogleCodeExporter]

From a naive search for 'synchronized [^{]*{[^{]*(return )?super[.]', I see no 
other instances of the problem in Guava, though I do see a few internal to 
Google in our stats code.

Original comment by cpov...@google.com on 13 May 2013 at 3:09

• Added labels: Package-EventBus

[GoogleCodeExporter]

I'm still waiting for the feedback from IBM, thanks for doing the search.

Original comment by dawid.weiss@gmail.com on 13 May 2013 at 4:13

[GoogleCodeExporter]

I still haven't heard from J9 folks. Anyway, checked guava's sources on git's 
master and this seems to be the only place that uses this pattern (there are 
others that call super methods from under synchronized blocks but they contain 
other code so I assume this cannot be optimized away).

Original comment by dawid.weiss@gmail.com on 14 May 2013 at 1:21

[GoogleCodeExporter]

Changed to a synchronized (this) block.

Original comment by cgdec...@gmail.com on 14 May 2013 at 7:47

• Changed state: Fixed
• Added labels: Type-Other

[GoogleCodeExporter]

This issue has been migrated to GitHub.

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

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

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

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

• Added labels: Restrict-AddIssueComment-Commit