backport: unchecked exceptions swallowed by executor service

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

backport: unchecked exceptions swallowed by executor service

Tom Sugden
Hi,

I was surprised by the behaviour that occurs when an unchecked exception is
raised by an overridden implementation of FutureTask#done() method. The
exception seems to be swallowed and the executor maintains a user thread for
some time afterwards. This isn't causing any problems for me at the moment,
but I was just wondering whether it was the intended behaviour? It could be
difficult to debug if your done() implementation had a programming error
that raised an unchecked exception.

A sample program is pasted below.

Cheers,
Tom

---

import edu.emory.mathcs.backport.java.util.concurrent.ExecutorService;
import edu.emory.mathcs.backport.java.util.concurrent.Executors;
import edu.emory.mathcs.backport.java.util.concurrent.FutureTask;

public class ExecutorTest
{
    public static void main(String[] args) throws Exception
    {
        ExecutorService executor = Executors.newCachedThreadPool();

        Runnable r = new Runnable()
        {
            public void run()
            {
                System.out.println("Running");
            }
        };
       
        FutureTask task = new FutureTask(r, null)
        {
            public void done()
            {
                System.out.println("Done!");
                throw new NullPointerException();
            }
        };

        executor.submit(task);
       
        task.get();
        System.out.println("End");
    }
}

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: backport: unchecked exceptions swallowed by executor service

tpeierls
The executor's worker thread hangs on for 60 seconds -- the default keepalive value for newCachedThreadPool() -- whether or not your task throws an exception in done(). Try commenting out the throw NPE line to see this.

You want to shutdown the executor if you are done with it. The standard way to do this in a main method is:

ExecutorService exec = ...;
try {
    ... do stuff with exec ...
} finally {
    exec.shutdown();
    if (!exec.awaitTermination(TIMEOUT, TIMEOUT_UNIT)) {
        System.err.printf("exec did not terminate after %d %s%n", TIMEOUT, TIMEOUT_UNIT);
    }
}

--tim


On 4/5/06, Tom Sugden <[hidden email]> wrote:
Hi,

I was surprised by the behaviour that occurs when an unchecked exception is
raised by an overridden implementation of FutureTask#done() method. The
exception seems to be swallowed and the executor maintains a user thread for
some time afterwards. This isn't causing any problems for me at the moment,
but I was just wondering whether it was the intended behaviour? It could be
difficult to debug if your done() implementation had a programming error
that raised an unchecked exception.

A sample program is pasted below.

Cheers,
Tom

---

import edu.emory.mathcs.backport.java.util.concurrent.ExecutorService;
import edu.emory.mathcs.backport.java.util.concurrent.Executors ;
import edu.emory.mathcs.backport.java.util.concurrent.FutureTask;

public class ExecutorTest
{
    public static void main(String[] args) throws Exception
    {
        ExecutorService executor = Executors.newCachedThreadPool ();

        Runnable r = new Runnable()
        {
            public void run()
            {
                System.out.println("Running");
            }
        };

        FutureTask task = new FutureTask(r, null)
        {
            public void done()
            {
                System.out.println("Done!");
                throw new NullPointerException();
            }
        };

         executor.submit(task);

        task.get();
        System.out.println("End");
    }
}

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: backport: unchecked exceptions swallowed by executor service

Andrew Lentvorski
In reply to this post by Tom Sugden
Tom Sugden wrote:
> Hi,
>
> I was surprised by the behaviour that occurs when an unchecked exception is
> raised by an overridden implementation of FutureTask#done() method. The
> exception seems to be swallowed and the executor maintains a user thread for
> some time afterwards. This isn't causing any problems for me at the moment,
> but I was just wondering whether it was the intended behaviour? It could be
> difficult to debug if your done() implementation had a programming error
> that raised an unchecked exception.

I got burnt by something similar.  The big problem is that you get into
the habit of assuming that unchecked exceptions will always squawk when
programming Java.  That fails when you start using concurrent.

I understand *why* they needed to use Exception, but, man, the first
time you get burnt by a NullPointerException which silently shuts down
your Callable you pull your hair out.

I now wrap all code that might let exceptions into Executor/Callables
like so:

> public Object captiveCall() throws InterruptedException, IOException {
> return obj;
> }
>
> public Object call() throws Exception {
> Object retval = null;
> try {
> retval = captiveCall();
> } catch (InterruptedException e) {
> l4j.debug("InterruptedException", e);
> throw e;
> } catch (IOException e) {
> l4j.debug("IOException", e);
> throw e;
> } catch (Exception e) {
> // This normally bad code of catch on Exception is here for a *reason*.
> // Future *eats* all exceptions *silently*.  This clause at least allows
> // the exception to emit noise for debugging.  This is particularly pernicious
> // if you have something like a NullPointerException
> l4j.debug("Exception", e);
> throw e;
> }
>
> return retval;
> }

-a
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: backport: unchecked exceptions swallowed by executor service

Gregg Wonderly-3
Andrew Lentvorski wrote:

> I now wrap all code that might let exceptions into Executor/Callables
> like so:
>
>>         public Object captiveCall() throws InterruptedException,
>> IOException {
>>             return obj;
>>         }
>>        
>>         public Object call() throws Exception {
>>             Object retval = null;
>>             try {
>>                 retval = captiveCall();
>>             } catch (InterruptedException e) {

This is an important pattern to utilize.  It allows you to focus your exception
processing for your application, and then if you make the captiveCall method
public, other applications that need different exception processing can do that.
  I've done this for quite some time, and it is nice to see this issue come up
on this list so that others can see this concept.

Gregg Wonderly
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: backport: unchecked exceptions swallowed by executor service

Joe Bowbeer
In reply to this post by Andrew Lentvorski
On 5/2/06, Andrew Lentvorski <[hidden email]> wrote:
>
> I now wrap all code that might let exceptions into Executor/Callables
> like so:

FutureTask can also be thought of as a "captive" Callable.  And
captive Runnable as well (don't forget the unchecked exceptions...).

The standard FutureTask isn't as proactive as your creation when it
comes to reporting the exceptions, though.  It relies on someone to
check the result and deal with the exception at that time.

If you're not checking the results of the task explicitly, the I would
still recommend (even in light of Tom Sugden's message) overriding
done() to handle exceptions proactively.

=> Then, if you want to go one step further, to report exceptions
thrown in done(), for example, you can override the afterExecute
method of the ThreadPoolExecutor.


On 5/2/06, Andrew Lentvorski <[hidden email]> wrote:

> Tom Sugden wrote:
> > Hi,
> >
> > I was surprised by the behaviour that occurs when an unchecked exception is
> > raised by an overridden implementation of FutureTask#done() method. The
> > exception seems to be swallowed and the executor maintains a user thread for
> > some time afterwards. This isn't causing any problems for me at the moment,
> > but I was just wondering whether it was the intended behaviour? It could be
> > difficult to debug if your done() implementation had a programming error
> > that raised an unchecked exception.
>
> I got burnt by something similar.  The big problem is that you get into
> the habit of assuming that unchecked exceptions will always squawk when
> programming Java.  That fails when you start using concurrent.
>
> I understand *why* they needed to use Exception, but, man, the first
> time you get burnt by a NullPointerException which silently shuts down
> your Callable you pull your hair out.
>
> I now wrap all code that might let exceptions into Executor/Callables
> like so:
>
> >               public Object captiveCall() throws InterruptedException, IOException {
> >                       return obj;
> >               }
> >
> >               public Object call() throws Exception {
> >                       Object retval = null;
> >                       try {
> >                               retval = captiveCall();
> >                       } catch (InterruptedException e) {
> >                               l4j.debug("InterruptedException", e);
> >                               throw e;
> >                       } catch (IOException e) {
> >                               l4j.debug("IOException", e);
> >                               throw e;
> >                       } catch (Exception e) {
> >                               // This normally bad code of catch on Exception is here for a *reason*.
> >                               // Future *eats* all exceptions *silently*.  This clause at least allows
> >                               // the exception to emit noise for debugging.  This is particularly pernicious
> >                               // if you have something like a NullPointerException
> >                               l4j.debug("Exception", e);
> >                               throw e;
> >                       }
> >
> >                       return retval;
> >               }
>
> -a

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: backport: unchecked exceptions swallowed by executor service

tpeierls
In reply to this post by Andrew Lentvorski
Please note that implementations of Callable may use more restrictive throws signatures than "throws Exception". If your Callable only throws IE and IOE, then you can just say so:

public class MyCallable<T> implements Callable<T> {
    public T call() throws InterruptedException, IOException { ... }
}

Exceptions thrown during execution are not eaten, they are rethrown by Future.get, wrapped in an ExecutionException.

And as Joe said, talking specifically about Tom Sugden's FutureTask.done issue, you can also override afterExecute to get common exception handling for a thread pool.

--tim


On 5/2/06, Andrew Lentvorski <[hidden email]> wrote:
Tom Sugden wrote:
> Hi,
>
> I was surprised by the behaviour that occurs when an unchecked exception is
> raised by an overridden implementation of FutureTask#done() method. The
> exception seems to be swallowed and the executor maintains a user thread for
> some time afterwards. This isn't causing any problems for me at the moment,
> but I was just wondering whether it was the intended behaviour? It could be
> difficult to debug if your done() implementation had a programming error
> that raised an unchecked exception.

I got burnt by something similar.  The big problem is that you get into
the habit of assuming that unchecked exceptions will always squawk when
programming Java.  That fails when you start using concurrent.

I understand *why* they needed to use Exception, but, man, the first
time you get burnt by a NullPointerException which silently shuts down
your Callable you pull your hair out.

I now wrap all code that might let exceptions into Executor/Callables
like so:

>               public Object captiveCall() throws InterruptedException, IOException {
>                       return obj;
>               }
>
>               public Object call() throws Exception {
>                       Object retval = null;
>                       try {
>                               retval = captiveCall();
>                       } catch (InterruptedException e) {
>                               l4j.debug("InterruptedException", e);
>                               throw e;
>                       } catch (IOException e) {
>                               l4j.debug("IOException", e);
>                               throw e;
>                       } catch (Exception e) {
>                               // This normally bad code of catch on Exception is here for a *reason*.
>                               // Future *eats* all exceptions *silently*.  This clause at least allows
>                               // the exception to emit noise for debugging.  This is particularly pernicious
>                               // if you have something like a NullPointerException
>                               l4j.debug("Exception", e);
>                               throw e;
>                       }
>
>                       return retval;
>               }

-a
_______________________________________________
Concurrency-interest mailing list
[hidden email]
<a href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)"> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest