Community
Participate
Working Groups
Exceptions are logged when assertions are enabled, but this causes us to miss important bugs when running in production environment. The solution is pretty simple: enable the use of a runner wrapper even when assertions are enabled, but this adds overhead to every runnable that's executed. It would be nice to run some performance tests to determine exactly what this performance penalty would be, before making this change.
Another option would be to revisit the question whether DefaultDsfExecutor should extend the ScheduledThreadPoolExecutor or just the ThreadPoolExecutor. We would loose the scheduling capability, but then we would gain on performance and we would not need to use a runnable wrapper to log the exceptions.
I browsed through the java Executor code to find out whether exceptions are recorded somewhere and it seems to me that overriding java.util.concurrent.ThreadPoolExecutor.afterExecute(Runnable, Throwable) by DefaultDsfExecutor would be an option to log uncaught exceptions without extra performance impact.
Created attachment 104831 [details] Override afterExecute Log exceptions from afterExecute. The beforeExecute method could be used for the tracing.
(In reply to comment #2) If DefaultDsfExecutor extended ThreadPoolExecutor that would indeed work. Unfortunately, it extends the SchedulingThreadPoolExecutor which itself wrappers every runnable and callable in ScheduledFutureTask. ScheduledFutureTask swallows exceptions generated by the runnables it wrappers and as a side effect afterExecute() is never called with the exception that was thrown. We could still override afterExecute() and cast the runnable to FutureTask and call get() on it to see if it throws ExecutionException. The problem with that is that when a Callable is submitted to the executor, a runtime exception may actually be thrown as part of regular execution flow, in which case we wouldn't want to log it. This is not the best practice in programming, but we do have a use case of that in the implementation of MIInferiorProcess.exitCode(). This is why I raised the option of rebasing DefaultDsfExecutor on ThreadPoolExecutor. I also thought of actually re-implementing SchedulingThreadPool executor, but it's a lot of work and it'd be easy to run into copyright issues :-(
Comment on attachment 104831 [details] Override afterExecute (In reply to comment #4) Got it now. Very inconvenient. Thanks for the explanation!
Attaching a patch which enables DefaultDsfExecutor to print a stacktrace in case any DsfRunnable throws a runtime exception. Users can also handle these exceptions meaningfully in the executor thread by passing an instance of ExceptionRequestMonitor to the execute() method. The original execute(Runnable) method will create a default exceptionrequestmonitor which just prints stacktrace. Two more variations of this method allow user to pass a parent request monitor, or a custom exceptionrequestmonitor. The default behaviour of printing stacktrace is required because of use cases like the one below: 1. Create a subclass of FinalLaunchSequence and override getSteps() to add the name of a step. 2. Do not actually add a method by that name to FinalLaunchSequence. 3. The method ReflectionSequence.getGroupSteps() will throw a runtimeexception which needs to be printed to console, along with stacktrace. Using the new DefaultDsfExecutor.execute() methods is pretty simple: 1. public void execute(Runnable command) - This is enhanced to just print stacktrace of runtime exceptions which occur in the runnable. 2. public void execute(Runnable command, RequestMonitor rm) - Just pass the parent requestmonitor along with runnable. This should not be the request monitor attached to the runnable, rather its parent monitor, because the exception requestmonitor is a sibling of the normal request monitor. If there is a runtime exception, the done() method of the normal request monitor will not be called, so completing the parent monitor is left upto the exception monitor. 3. public void execute(Runnable command, ExceptionRequestMonitor erm) - Here user can also customize the exception handling beyond just printing stacktrace. P.S.: This is more of a simple hack to get stacktrace of runtime exceptions, rather than a full fledged solution for handling runtime exceptions.
Created attachment 190013 [details] Simple patch to print stacktrace for runtime exceptions
(In reply to comment #6) > P.S.: This is more of a simple hack to get stacktrace of runtime exceptions, > rather than a full fledged solution for handling runtime exceptions. I was rather hoping for a full fledged solution :-( Though we've been waiting for one for a long time and no one has stepped up. This solution is a variation on the TracingWrapper theme and perhaps that's what we'll have to settle for, though this one has an interesting twist. I like this idea of automatically propagating errors to the current outstanding request monitor. I've thought about this before, but I was trying for a way to make this linking automatic and that's probably too unrealistic. Have you considered what kind of effort it would take to refactor the existing code to take advantage of this new API? BTW, one alternative fix for this bug would be to remove the SchedulingThreadPoolExecutor from the DsfExecutor interface hierarchy. It would be an API-breaking change, but the real fallout would probably be rather small.
(In reply to comment #8) > BTW, one alternative fix for this bug would be to remove the > SchedulingThreadPoolExecutor from the DsfExecutor interface hierarchy. It > would be an API-breaking change, but the real fallout would probably be rather > small. A quick check revealed that it's probably less work (and less cowardly) to reimplement than to remove the Scheduling feature.
