[JIRA] Created: (MDA-44) exceptions should be propagated up from subordinate pipelines

[Ian Young (JIRA)]

exceptions should be propagated up from subordinate pipelines
-------------------------------------------------------------

                 Key: MDA-44
                 URL: https://issues.shibboleth.net/jira/browse/MDA-44
             Project: Metadata Aggregator
          Issue Type: Bug
          Components: Pipeline
    Affects Versions: 0.6
            Reporter: Ian Young
            Assignee: Chad La Joie


If an exception (in my case, a custom sub-class of StageProcessingException) is thrown within a stage executed in the top-level pipeline (invoked by the CLI) then the exception causes CLI termination as expected.  If the same exception is thrown within a pipeline invoked from the main pipeline by (e.g.) PipelineDemultiplexerStage then the exception is logged as an error instead of being propagated, and thus CLI termination does not occur.

I suspect the same thing would happen for other ways of invoking subordinate pipelines, e.g., PipelineMergeStage, SplitMergeStage.

I think the proximate cause of this is the catch in PipelineCallable.call(), which catches any PipelineProcessingException, logs it and returns without re-throwing.  Chad thinks this is a hangover from when this was implementing Runnable rather than Callable, as Runnable doesn't allow the invoked method to throw anything.  So fixing this part of the problem is probably as simple as declaring that call() throws PipelineProcessingException and removing the try/catch altogether.  I guess retaining the try/catch, demoting the log.error to prevent duplicate output and re-throwing the exception would be acceptable too, but I don't think there's much point in the extra complication.

I think this change would just move the issue up one level, though.  As I understand it, any exception thrown within a Future.get()s evaluation is re-thrown as a java.concurrent.ExecutionException.  It looks like this wrapped exception is being caught in the doExecute methods of the three stages listed above and (again) logged without being propagated in any way.  To propagate things correctly, the caught ExecutionException's cause needs to be examined to see if it is a StageProcessingException and if so re-thrown rather than logged.  Something like this perhaps:

    try {
        pipelineFuture.get();
    } catch (ExecutionException e) {
        if (e.getCause() instanceof StageProcessingException) {
           throw e.getCause();
        }
        log.error("Pipeline threw an unexpected exception", e);
    } catch (InterruptedException e) {
        log.error("Execution service was interrupted", e);
    }

On the other hand, I'm not sure that I wouldn't want *any* kind of ExecutionException to be regarded as a kind of StageProcessingException, in which case re-throwing all ExecutionExceptions as wrapped StageProcessingExceptions might be more appropriate:

    try {
        pipelineFuture.get();
    } catch (ExecutionException e) {
        throw new StageProcessingException("Pipeline threw an unexpected exception", e);
    } catch (InterruptedException e) {
        log.error("Execution service was interrupted", e);
    }

I think I'd lean towards this latter option if only because it was cleaner.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira