[
https://issues.apache.org/jira/browse/ISIS-948?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dan Haywood updated ISIS-948:
-----------------------------
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.
{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
was:
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.
[1] http://isis.markmail.org/thread/o5zbm54cpb4tizqt
> 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)
