Skip to content

issue #1992 redirect status to log service if threading service has shut down #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

LeeKamentsky
Copy link
Member

This patch catches a RejectedExecutionException that is thrown by the threading service after it has shut down. The StatusService tries to enqueue a status message during shutdown, but the threading service isn't there any more to pass it onto the event thread. I catch the exception and redirect the message to logging - I think that's appropriate at that point since there's no UI to display the status.

@ctrueden
Copy link
Member

The SciJava Context disposes services in reverse order; that is, downstream services should be disposed before their dependencies. In this way, it should not be possible for a service to "not know" it has been disposed and find out that one of its dependencies has gone kaput.

So I think a better solution here is to override the void dispose() method inside DefaultEventService so that the event service knows it has been disposed, and then if someone tries to publish an event with that service, it simply ignores the request.

@ctrueden
Copy link
Member

As a first cut (untested), something like:

diff --git a/src/main/java/org/scijava/event/DefaultEventService.java b/src/main/java/org/scijava/event/DefaultEventService.java
index f8527e9..8121b5f 100644
--- a/src/main/java/org/scijava/event/DefaultEventService.java
+++ b/src/main/java/org/scijava/event/DefaultEventService.java
@@ -91,12 +91,14 @@ public class DefaultEventService extends AbstractService implements

        @Override
        public <E extends SciJavaEvent> void publish(final E e) {
+               if (eventBus == null) return;
                e.setContext(getContext());
                eventBus.publishNow(e);
        }

        @Override
        public <E extends SciJavaEvent> void publishLater(final E e) {
+               if (eventBus == null) return;
                e.setContext(getContext());
                eventBus.publishLater(e);
        }
@@ -105,12 +107,13 @@ public class DefaultEventService extends AbstractService implements
        public List<EventSubscriber<?>> subscribe(final Object o) {
                final List<EventSubscriber<?>> subscribers =
                        new ArrayList<EventSubscriber<?>>();
-               subscribeRecursively(subscribers, o.getClass(), o);
+               if (eventBus != null) subscribeRecursively(subscribers, o.getClass(), o);
                return subscribers;
        }

        @Override
        public void unsubscribe(final Collection<EventSubscriber<?>> subscribers) {
+               if (eventBus == null) return;
                for (final EventSubscriber<?> subscriber : subscribers) {
                        unsubscribe(subscriber);
                }
@@ -120,6 +123,7 @@ public class DefaultEventService extends AbstractService implements
        public <E extends SciJavaEvent> List<EventSubscriber<E>> getSubscribers(
                final Class<E> c)
        {
+               if (eventBus == null) return null;
                // HACK - It appears that EventBus API is incorrect, in that
                // EventBus#getSubscribers(Class<T>) returns a List<T> when it should
                // actually be a List<EventSubscriber<T>>. This method works around the
@@ -144,6 +148,7 @@ public class DefaultEventService extends AbstractService implements
        @Override
        public void dispose() {
                eventBus.clearAllSubscribers();
+               eventBus = null;
        }

        // -- Helper methods --

@LeeKamentsky
Copy link
Member Author

It was a bit of a shock to see the exception thrown, but it was diagnostic. A java.util.concurrent.RejectedExecutionException was thrown and maybe it's up to the caller to figure out how to react to that. If we silence the event service, the status service won't be able to take an alternative action when the exception is thrown.

I think this is your call though - I'm not so much of an active developer at this point and there is some merit to silently failing to send events as the system is shutting down. If you accept my patch, it will take care of this instance of the problem and won't cause any harm going forward.

@bdezonia
Copy link
Contributor

I've created this ticket to track this issue:

http://trac.imagej.net/ticket/2013

@ctrueden
Copy link
Member

After further consideration, I fixed the problem this way: e24e624.

@ctrueden ctrueden closed this Oct 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants