Balzanka / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Make EventBus subclass friendly #1348

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
The current Eventbus implementation has all methods package-private with the 
exception of register, unregister & post. Ideally either all other methods 
should be protected, or if that's not desired, EventBus should be an interface 
(because currently you'd have to rewrite evertyhing anyway when subclassing 
EventBus as nothing is accessible from the subclass).

Original issue reported on code.google.com by DeRijcke.Erik@gmail.com on 21 Mar 2013 at 8:58

GoogleCodeExporter commented 9 years ago
Do you have a specific use case?  I am fairly certain this is working as 
intended.

Original comment by wasserman.louis on 21 Mar 2013 at 3:43

GoogleCodeExporter commented 9 years ago
Issue 1349 is a good example. Tthere is now way for me to implement this 
functionality without c/p EventBus internals. Currenlty I have to extend 
EventBus and c/p it's implementation. Not only is this impossible if I already 
extend another class, it's also bad practice to c/p code...

If for some reason it is needed that nothing of EventBus internals should be 
exposed outside its package, then EventBus should be an interface with 
register, unregister and post method to allow for maximum flexibility (ie 
multiple inheritance), as currently extending EventBus is exactly the same as 
implementing an interface (because nothing is accessible from the 
implementation).

Original comment by DeRijcke.Erik@gmail.com on 21 Mar 2013 at 7:40

GoogleCodeExporter commented 9 years ago
TBH, historical precedent is that you *should* go ahead and fork EventBus.  
There are several forks out there already.

IIRC, the reasoning, for what it's worth, is that
a) we might like to change EventBus semantics in the future, or add new 
methods, that would break all users of the interface
b) once we open up EventBus for extension, we now have to preserve backwards 
compatibility of the implementation for all those extensions

Original comment by wasserman.louis on 21 Mar 2013 at 10:09

GoogleCodeExporter commented 9 years ago
a) Not if users can extend from a subclass friendly base implementation...

Users of the interface (clients), they will always brake if you do an api 
incompatible change, regardless if it's an interface or class. As for 
implementers of the interface, they will not brake at runtime if it's 
compatible api change (ie add a method), only when an api client actually calls 
the new method it will and should brake.

 This brings me to a an extra argument to use an interface instead of a class. If somebody extends EventBus class, overrides all public methods and one day an extra method is added to EventBus, then users of the extended EventBus have no clue the extra added method is actually not implemented/overridden. The program just keeps on running without error. Try debugging that...

b) well you are building a software library... api compatability is always a 
concern.

Original comment by DeRijcke.Erik@gmail.com on 24 Mar 2013 at 5:28

GoogleCodeExporter commented 9 years ago
If you look through the other feature requests for EventBus, you'll see things 
like custom exception handling that we couldn't compatibly add if any 
subclassing was allowed, whether of an interface or a class.  (We could 
compatibly add it ourselves, by making sure the default was consistent with the 
current behavior.)  

Disallowing subclasses entirely allows us the flexibility to add that sort of 
thing.

There are several known forks of EventBus out there, and it's not unreasonable 
at all to do your own.

Original comment by wasserman.louis on 24 Mar 2013 at 6:43

GoogleCodeExporter commented 9 years ago
Could you explain? Subclassing EventBus is currently allowed, although only 
through class extension. Are there plans to make EventBus a final?

I agree, it's not unreasonable to do it all yourself, but that brings me back 
to the argument of extracting EventBus public methods as an interface (=add a 
"Bus" interface that is implemented by EventBus) as stated before:

"If somebody extends EventBus class, overrides all public methods and one day 
an extra method is added to EventBus, then users of the extended EventBus have 
no clue the extra added method is actually not implemented/overridden. The 
program just keeps on running without error. Try debugging that."

Original comment by DeRijcke.Erik@gmail.com on 24 Mar 2013 at 7:56

GoogleCodeExporter commented 9 years ago
Use Case:
- Developers forget to add @AllowConcurrent which leads to huge performance 
problems.

I wanted to change the default strategy but I could not because it was static 
final in the EventBus class. Overriding register and unregister didn't work 
because there's no access to handlersByType.. because its private and there's 
no protected to add handlers for it..

So I had to override pretty much the whole package..

Original comment by wdro...@droste-usa.com on 20 Nov 2013 at 10:02

GoogleCodeExporter commented 9 years ago
Another use case:
We're using the eventBus synchronously to logically decouple our code. When an 
exception occurs in an event handler, we want the exception to be thrown back 
to the object posting the event. 

In the current implementation, any exception is swallowed in eventBus.dispatch. 
A simple override of this method could do the trick, but the method is not 
protected.

Original comment by guterfl...@gmail.com on 4 Dec 2013 at 12:08

GoogleCodeExporter commented 9 years ago
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

GoogleCodeExporter commented 9 years ago

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

GoogleCodeExporter commented 9 years ago

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