[
https://issues.apache.org/jira/browse/ISIS-948?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dan Haywood resolved ISIS-948.
------------------------------
Resolution: Fixed
> Fix concurrent exception in EventBus, improve support for request-scoped
> services
> ---------------------------------------------------------------------------------
>
> Key: ISIS-948
> URL: https://issues.apache.org/jira/browse/ISIS-948
> Project: Isis
> Issue Type: Improvement
> Components: Core
> Affects Versions: core-1.7.0
> Reporter: Dan Haywood
> Assignee: Dan Haywood
> Fix For: core-1.8.0
>
>
> 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.
> {code:java}
> @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);
> }
> }
> {code}
> 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.
> [1] http://isis.markmail.org/thread/o5zbm54cpb4tizqt
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
