Details
-
Improvement
-
Status: Closed
-
Major
-
Resolution: Fixed
-
core-1.7.0
-
None
Description
This ticket was prompted originally by [1], flagging up a concurrency issue.
The background here is that EventBusService (a singleton) manages the registered subscribers for the guava EventBus, but the EventBus itself is stored internally by the framework on IsisSession, ie is in effect request-scoped. There are some special-case hooks to call open() and close() on EventBusService when the xactn is started/completed so that the service can call register() on the guava EventBus object for each interaction.
The concurrency exception is (I think) because the set used in EventBusService was not thread-safe; should've been ConcurrentHashSet or similar.
~~~
Thinking about this issue has led me to consider whether the guava EventBus should have been singleton rather than request-scoped, in particular with respect to whether we can accommodate request-scoped subscriber services as well as singleton services. Have come to the conclusion that it doesn't actually matter either way; it would be safe to have a singleton event bus having request-scoped services as subscribers because the proxies for those services would always route the method call to the on(Event) to the appropriate underlying service from its proxy's threadlocal.
That said, have decided to keep the EventBus request-scoped. But rather than have all the special case logic, instead wrap the guava EventBus in a new RequestScopedEventBus domain service (as its name suggests, is @RequestScoped) and have this act as the container of the guava EventBus and also keep track of the subscribers.
For its part, the original EventBusService remains as a singleton whose job it is to keep track of all the subscribers. Some of these are singletons that will register in the system bootstrapping (during their @PostConstruct lifecycle). Some of these might also be request-scoped services. So far as possible want the model to be the same... that they can register in their own @PostConstruct lifecycle. Now for these request-scoped services, the @PostConstruct should actually be called after injection of the EventBusService... The one slight modification is that they should not register themselves but instead their proxy. They can get hold of their proxy simply by having it be injected:
eg.
@RequestScoped @DomainService public class MySubscribingService { private EventBusService ebs; public void injectEventBusService(EventBusService ebs) { this.ebs = ebs; } private MySubscribingService proxy; public void injectProxy(MySubscribingService proxy) { this.proxy = proxy; } @PostConstruct public void startRequest() { ebs.register(proxy); } @PreDestroy public void endRequest() { ebs.unregister(proxy); } }
Note also that we now allow injection into request-scoped services (though must use injectXxx rather than @Inject xxx).
In the EventBusService, because request-scoped services might be continually registering and unregistering, but also each will be registering their proxy, then this needs to change to do reference counting. Thus, if there are two concurrent threads then the MySubscribingService (above) will be instantiated twice, each held in the thread-local of the proxy. This proxy will (in the @PostConstruct) be registered with the EventBusService twice (for each thread), but of course that proxy should only be registered with the guava EventBus once.
The RequestScopedEventBus is responsible for instantiating the guava EventBus, but it does this lazily, when the first call to post(...) is made. At this time it calls back to the EventBusService asking for the current subscribers. The EventBusService will return all the singletons (because they will always have a reference count = 1) plus it will return the proxy of all the subscribing request-scoped services (because there will always at least one current thread that would've caused its proxy to have been registered).
As I say, an alternative design could probably have dispensed with the reference counting and having the guava EventBus being request-scoped; this simpler design would instead simply hold a set of subscribers in EventBusService. The singleton subscribing services would all be added during bootstrap, while the (singleton) proxy for all the request-scoped subscribing services would be added in the very first transaction.
Might end up refactoring to this simpler design as and when we bring CDI into the mix.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
UPDATE: as the commits below perhaps indicate, in the end I've decided to go the extra step and to refactor the design so that the guava EventBus is a singleton, held by EventBusService (and new'd up by the EventBusServiceDefault subclass). This removes lots of complexity at the expense of a no-longer fully symmetric design:
- the event bus is new'd up only when needed, NOT in the @PostConstruct of the EventBusService
- request-scoped services get re-registered for each subsequent xactn, and these additional re-registrations are simply ignored.
- request-scoped services must NOT unregister themselves. In fact, the EventBusService's unregister is now a no-op.
Have also added in some checks to ensure that request-scoped services only ever register their proxy.
Attachments
Issue Links
- is duplicated by
-
-
- Closed
-