Exception in run() break PoolFiber

[GoogleCodeExporter]

For reproducing must throw RuntimeException from run() method in PoolFiber.

Part of stack trace see as below:

at org.jetlang.channels.ChannelSubscription$1.run(ChannelSubscription.java:31)
        at org.jetlang.core.BatchExecutorImpl.execute(BatchExecutorImpl.java:11)
        at org.jetlang.fibers.PoolFiber.flush(PoolFiber.java:63)
        at org.jetlang.fibers.PoolFiber.access$000(PoolFiber.java:19)
        at org.jetlang.fibers.PoolFiber$1.run(PoolFiber.java:36)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
        at java.lang.Thread.run(Thread.java:722)

The problem is that if an exception will be throwed, then value of the 
_flushpending never will be changed.

In source code (in org.jetlang.fibers.PoolFiber.flush() at 61): 
  private void flush() {
        buffer = _queue.swap(buffer);
        _commandExecutor.execute(buffer); // exception throwed here
        buffer.clear();

        _flushPending.compareAndSet(true, false); // here state never be changed

        if (!_queue.isEmpty()) {
            flushIfNotPending();
        }
    }

Obvious fix:

     private void flush() {
         buffer = _queue.swap(buffer);
+        try {
             _commandExecutor.execute(buffer);
             buffer.clear();

+        } finally {
             _flushPending.compareAndSet(true, false);
+        }

         if (!_queue.isEmpty()) {
             flushIfNotPending();

But part of 'buffer' content may be lost.

Original issue reported on code.google.com by radio...@ya.ru on 13 Feb 2012 at 7:34

[GoogleCodeExporter]

Thanks for the catch. I hadn't noticed this as I use a custom BatchExecutor 
implementation that manages exceptions on a fiber.

Original comment by peter.royal@pobox.com on 14 Feb 2012 at 1:25

• Changed state: Accepted

[GoogleCodeExporter]

Looking at your suggested fix, if _commandExecutor is given a buffer with 5 
Runnables, and the first one fails, the other 4 will never be run.

This seems bad as well. I'm not sure how to comprehensively address other than 
by using a custom BatchExecutor that implements your exception handling policy

Original comment by peter.royal@pobox.com on 14 Feb 2012 at 1:27

[GoogleCodeExporter]

Yes, i known that my fix is no good.

May be a best way it add error handler (like 
java.lang.Thread.UncaughtExceptionHandler) and  delegate him exception - to 
write in log, or other framework user action.

Error handler may be passed into BatchExecutorImpl:

public class BatchExecutorImpl implements BatchExecutor {
    private final Errorback errorback;

    public void execute(EventReader toExecute) {
        for (int i = 0; i < toExecute.size(); i++) {
            try {
                Runnable runnable = toExecute.get(i);
                runnable.run();
            } catch(Throwable t) {
                errorback.error(runnable, t);
            }
        }
    }
}

Original comment by radio...@ya.ru on 14 Feb 2012 at 6:04

[GoogleCodeExporter]

I second this - misbehaving code is simply a fact of life in larger systems, 
and having my actor framework stop working because one component threw an 
exception is tricky.

I would prefer something like an UncaughtExceptionHandler interface that I can 
set, with the default behaviour being to catch the exception, print it to 
stdout, and continue processing events.

Original comment by noelgrandin on 23 Feb 2012 at 10:08