> On April 27, 2015, 8 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java, line 71
> > <https://reviews.apache.org/r/33456/diff/1/?file=940089#file940089line71>
> >
> >     Have you considered creating factory methods that apply decorators to 
> > ExecutorServices?  That would potentially save this code from the 
> > combinatoric explosion we seem to be heading towards.  
> > ForwardingExecutorService [1] could be helpful to minimize the code to 
> > implement decorators.
> >     
> >     [1] 
> > http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/ForwardingExecutorService.html
> 
> Maxim Khutornenko wrote:
>     I could not figure out how to make it useful for this particular case. 
> The `afterExecute` is defined in a more concrete ThreadPoolExecutor rather 
> than in ExecutorService that ForwardingExecutorService implements. The 
> implementation does not leave a second chance to handle unhandled error 
> (ThreadPoolExecutor.runWorker):
>     ```
>                     try {
>                         beforeExecute(wt, task);
>                         Throwable thrown = null;
>                         try {
>                             task.run();
>                         } catch (RuntimeException x) {
>                             thrown = x; throw x;
>                         } catch (Error x) {
>                             thrown = x; throw x;
>                         } catch (Throwable x) {
>                             thrown = x; throw new Error(x);
>                         } finally {
>                             afterExecute(task, thrown);
>                         }
>                     } finally {
>                         task = null;
>                         w.completedTasks++;
>                         w.unlock();
>                     }
>     ```
> 
> Bill Farner wrote:
>     Darn.  Thinking more broadly about this, it's surprising that we're 
> getting no logging at all.  We should at least have a default uncaught 
> exception handler installed that logs [1].
>     
>     Thinking from that direction, it's obvious why this is happening.  We 
> create these thread pools before the uncaught exception handler is installed. 
>  I suggest we fix that instead - define a custom `UncaughtExceptionHandler`, 
> and call `Thread.setDefaultUncaughtExceptionHandler()` as the first order of 
> business in `main()` [2].
>     
>     
>     [1] 
> https://github.com/twitter/commons/blob/9fc05c8cc927ade33cb14ee21d433669af218c91/src/java/com/twitter/common/application/Lifecycle.java#L48
>     [2] 
> https://github.com/apache/aurora/blob/65df91bfd7e3a2ada38a5fe4d620e6373d0f59bf/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L263
> 
> Bill Farner wrote:
>     Oh wait, i forgot that does not fully address the problem when the 
> ExecutorService catches the exception internally.  We should still address 
> the above issue, but it's clearly not a solution by itself.
> 
> Bill Farner wrote:
>     Going back to my first suggestion above, can you not wrap the incoming 
> Runnables and Callables?
>     
>     For example, the `ExecutorService` decorator might look like this:
>     ```
>       public static ExecutorService logExceptions(final ExecutorService 
> service) {
>         return new ForwardingExecutorService() {
>           @Override
>           protected ExecutorService delegate() {
>             return service;
>           }
>     
>           private Runnable logExceptions(final Runnable command) {
>             return new Runnable() {
>               @Override
>               public void run() {
>                 try {
>                   command.run();
>                 } catch (RuntimeException e) {
>                   // Log.
>                 }
>               }
>             };
>           }
>     
>           @Override
>           public void execute(Runnable command) {
>             super.execute(logExceptions(command));
>           }
>         };
>       }
>     ```
>     
>     There's a bunch of methods to override, but `logExceptions` and a 
> counterpart for `Callable` would be reusable, and we don't have have the 
> factory method explosion.

This may work for the ExecutorService but will not wrap the 
ScheduledExecutorService.submit(), which is defined in downstream.

This is the first and likely the last entry in this class as we now have both 
scheduled and regular executors represented here, which seems to cover all our 
use cases. So, I am not sure I share your concern about combinatorial explosion.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33456/#review81725
-----------------------------------------------------------


On April 22, 2015, 10:58 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33456/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 10:58 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Our async EventBus is using a regular executor thus potentially hiding 
> unhandled errors.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/AsyncUtil.java 
> f657e057b5bbff69971876e104ff0e47b2dc4faa 
>   src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 
> 3a4d40adc1abe170b5b80644db9f079751d8a9bf 
>   src/test/java/org/apache/aurora/scheduler/base/AsyncUtilTest.java 
> e990f528aac768b5c9b829c9544045a831e094fe 
> 
> Diff: https://reviews.apache.org/r/33456/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to