Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-35321

MavenModuleSetBuild swallows InterruptedIOException on doRun

    • Icon: Improvement Improvement
    • Resolution: Unresolved
    • Icon: Minor Minor
    • maven-plugin
    • None

      Definitely not a deal breaker. Just makes debugging hard

      https://github.com/jenkinsci/maven-plugin/blob/master/src/main/java/hudson/maven/MavenModuleSetBuild.java#L898

      catch (InterruptedIOException e) {
                      e.printStackTrace(listener.error("Aborted Maven execution for InterruptedIOException"));
                      return Executor.currentExecutor().abortResult();
                  }
      

      Propose adding message to printStackTrace. Something like:

      catch (InterruptedIOException e) {
                      e.printStackTrace(listener.error("Aborted Maven execution for InterruptedIOException: " + e.getMessage()));
                      return Executor.currentExecutor().abortResult();
                  }
      

          [JENKINS-35321] MavenModuleSetBuild swallows InterruptedIOException on doRun

          owood (cc olamy) I don't understand why we don't use classical loggers to record these stacktraces and we are using e.printStackTrace ?!?
          owood I'm not sure that only the message may help and we won't need the full stack but we can give it a try (it's cheap)

          Arnaud Héritier added a comment - owood (cc olamy ) I don't understand why we don't use classical loggers to record these stacktraces and we are using e.printStackTrace ?!? owood I'm not sure that only the message may help and we won't need the full stack but we can give it a try (it's cheap)

          Olivier Lamy added a comment -

          aheritier sounds bad IMHO using the logger sounds a better option (but with the interesting stack trace!)

          Olivier Lamy added a comment - aheritier sounds bad IMHO using the logger sounds a better option (but with the interesting stack trace!)

          Owen Wood added a comment -

          aheritier olamy Listener logger is existing functionality. I didn't add / change this. Happy to use classical logger technique, was trying to keep the code consistent. It seems a bit odd to me, but I don't have much XP with this plugin so shrugged it off.

          Would be for ripping out the current listener logging approach and replacing with just a standard logger approach but I believe that this is out of scope for what this was raised to address.

          Happy to raise a separate issue to address the logging implementation and having this depend on it. WDYT?

          Owen Wood added a comment - aheritier olamy Listener logger is existing functionality. I didn't add / change this. Happy to use classical logger technique, was trying to keep the code consistent. It seems a bit odd to me, but I don't have much XP with this plugin so shrugged it off. Would be for ripping out the current listener logging approach and replacing with just a standard logger approach but I believe that this is out of scope for what this was raised to address. Happy to raise a separate issue to address the logging implementation and having this depend on it. WDYT?

          Owen Wood added a comment -

          As for this implementation, it should probably just be something like this:

          e.printStackTrace();
          

          for now.

          But I implemented this:

          e.printStackTrace(listener.error("Aborted Maven execution for InterruptedIOException: " + ExceptionUtils.getStackTrace(e)));
          

          For the sake of over engineering Using Apache Commons ExceptionUtils.

          Its terrible, I agree.

          Owen Wood added a comment - As for this implementation, it should probably just be something like this: e.printStackTrace(); for now. But I implemented this: e.printStackTrace(listener.error( "Aborted Maven execution for InterruptedIOException: " + ExceptionUtils.getStackTrace(e))); For the sake of over engineering Using Apache Commons ExceptionUtils . Its terrible, I agree.

          owood yes it should be in another PR. I prefered to ask to olamy if he didn't know if there was a reason for this because this plugin may hide many fun stuffs

          Arnaud Héritier added a comment - owood yes it should be in another PR. I prefered to ask to olamy if he didn't know if there was a reason for this because this plugin may hide many fun stuffs

            owood Owen Wood
            owood Owen Wood
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: