Uploaded image for project: 'Metadata Aggregator'
  1. Metadata Aggregator
  2. MDA-44

exceptions should be propagated up from subordinate pipelines

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.0
    • Fix Version/s: 0.8.0
    • Component/s: Pipeline
    • Labels:
      None

      Description

      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.

        Attachments

          Activity

            People

            Assignee:
            ian@iay.org.uk Ian Young
            Reporter:
            ian@iay.org.uk Ian Young
            Watchers:
            1 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 1 hour, 53 minutes
                1h 53m