Exceptions during Service start

[pe...@knewton.com]

I have been investigating the behavior of guava Services when Exceptions are thrown at different points in their lifecycle.

If an exception is thrown while starting a guava Service (AbstractIdleService and AbstractScheduledService, haven't looked at others) the exception is thrown back to startAndWait() or start().get() correctly but the thread the service was starting up on also dies with an exception. In my use case, the startAndWait() exception is caught and dealt with but the other exception gets caught by an UncaughtExceptionHandler.

However, if an exception is thrown after startup by a Service that uses a separate Thread for execution, the thread dies without throwing an uncaught exception.

Shouldn't the behavior here at least be consistent? Did I miss something here? The current handling of exceptions after startup seems more correct to me.

Thanks,

Peter Kelley

[Chris Povirk]

I suspect that the lack of consistency is an oversight. I suspect that the original decision to rethrow exceptions (as you see in some Service classes) was probably the wrong one.

When you say "an exception is thrown after startup by a Service that uses a separate Thread for execution," I take it that you're looking at AbstractScheduledService? That class uses the Future-based methods of ExecutorService, and the Future methods catch exceptions. By contrast, AbstractExecutionThreadService uses Executor.execute(), and execute() doesn't catch exceptions. (Oddity: While we need to catch them manually there to update the service state, we then rethrow them. We even have tests for this.) As far as I can tell, they've both been that way from the beginning.

My initial reaction would be that it would be nice to standardize on catching the exception. My thinking is that UncaughtExceptionHandler is for cases in which you have no alternative way to handle an exception (or, optimistically, cases in which you haven't set up the alternative way yet). Service provides its own monitoring, so I don't see why we'd want to rethrow here. More generally, we're violating the principle that you either handle an exception or you don't -- none of this "I'll log it and then rethrow it for someone else to catch and log."

Maybe someone else has another perspective? I'll at least run some tests internally to see if anything obvious breaks with the change.

[Luke Sandberg]

I agree, notification of failed services shouldn't attempt to use uncaught exception handlers.

I know Greg Kick had other opinions in reviews over the years, but i think it may have just been the general prejudice against catching Throwable or Error.  But i think for Service (or Future) it is appropriate since there are dedicated alternate failure reporting mechanisms.

--
guava-...@googlegroups.com
Project site: https://github.com/google/guava
This group: http://groups.google.com/group/guava-discuss
 
This list is for general discussion.
To report an issue: https://github.com/google/guava/issues/new
To get help: http://stackoverflow.com/questions/ask?tags=guava
---
You received this message because you are subscribed to the Google Groups "guava-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to guava-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/guava-discuss/CAEvq2nqTLxtfoWtU_00O8cP8%2B39RCw1cOLNE2g5rWH80nmexKA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[pe...@knewton.com]

Thanks for looking at this. 

And yes, I was referring to AbstractScheduledService when considering how exceptions are handled after startup.

[Luke Sandberg]

specifically for AbstractScheduledService, if an exception is thrown from 'runOneIteration()' then subsequent executions are canceled and the failure() listeners are invoked.

One other point of difference between AbstractScheduledService and AbstractExecutionThreadService is that, when the run/runOneIteration method throws an exception.  AETS runs the shutDown() method but ASS does not... we should probably standardize that behavior as well.

To view this discussion on the web visit https://groups.google.com/d/msgid/guava-discuss/0c28ea2a-8e83-41fc-ae44-7b33eddee058%40googlegroups.com.

[Luke Sandberg]

This change has been pushed to github: https://github.com/google/guava/commit/c633d07f26e9859529e79e7da4057ad097d9f5f4

I am curious if anyone has a strong opinion on whether or not 'shutDown' should be called when a service transitions from RUNNING->FAILED.  AbstractExecutionThreadService does do this and AbstractScheduledService does not do this.

AbstractService also does not call 'doStop()' when a service transitions RUNNING->FAILED so I think it may make sense to remove this behavior from AbstractExecutionThreadService.  Thoughts?

[Tim Peierls]

I think it's OK to have inconsistencies in these behaviors, as long as they are clearly documented. AbstractExecutionService, in particular, already has the caveat, "Consider AbstractService, if you would like to manage any threading manually", which to me implies that it manages things more closely than its supertype.

But the details should be explicit in the javadocs.

--tim

To view this discussion on the web visit https://groups.google.com/d/msgid/guava-discuss/CAEu%3D3G%3DxuLAfDgKrFvvS6Zo-2Lqq%2BfCLdQKzvOfHesQe7xPgKA%40mail.gmail.com.

[Chris Povirk]

I am curious if anyone has a strong opinion on whether or not 'shutDown' should be called when a service transitions from RUNNING->FAILED.  AbstractExecutionThreadService does do this and AbstractScheduledService does not do this.

Glancing at a handful of AbstractExecutionThreadService callers inside Google, I get the impression that the behavior is probably a good thing for more users than it is a bad thing. (My main worry is over people whose shutDown() tries to flush pending data. I would hope that flushing after failure is already something they handle, but perhaps not.) Glancing at a handful of AbstractScheduledService callers, I see the same thing but maybe less so. A lot of users seem to be shutting down their executor.

AbstractService also does not call 'doStop()' when a service transitions RUNNING->FAILED so I think it may make sense to remove this behavior from AbstractExecutionThreadService.  Thoughts?

A hypothetical AbstractService.doStop() call would be located... in startAsync()? Or maybe more generally in notifyFailed()?