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

TeeOutputStream writes to second OutputStream twice

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Minor Minor
    • remoting
    • None
    • org.jenkins-ci.main:remoting:4.3@jar
    • 3148.v532a_7e715ee3

      The following simple reproduction demonstrates that the second OutputStream passed to hudons.remoting.TeeOutputStream gets data written to it twice (unexpected/bug), wheres the first OutputStream only gets it once (as expected). The issue is so simple, I have no idea how this isn't a known bug causing all sorts of issues, so I assume I'm missing something.  Although maybe it only affects local FilePaths, and maybe this ticket is related (was closed as can't reproduce):

      https://issues.jenkins.io/browse/JENKINS-38520

      Also of note, I switched the import to 
      import org.apache.commons.io.output.TeeOutputStream and it behaves as expected, (each stream/file getting messages once.)
       
       

      import hudson.remoting.TeeOutputStream  
      //import org.apache.commons.io.output.TeeOutputStream 
      
      File file = new File("test.txt")
      File file2 = new File("test2.txt")
      OutputStream os = new FileOutputStream(file)
      OutputStream os2 = new FileOutputStream(file2)
      TeeOutputStream tos = new TeeOutputStream(os, os2)
      PrintWriter writer = tos.newPrintWriter()
      writer.write("testing ")
      writer.flush()
      assert file.getText() == "testing "
      assert file2.getText() == "testing testing "

       

          [JENKINS-71856] TeeOutputStream writes to second OutputStream twice

          Mark Waite added a comment - - edited

          Thanks for the clear description of the issue. I used Jenkins 2.414.1-rc and confirmed that the Jenkins script console shows that file2 in your example receives two copies of the testing string. When the import is replaced with org.apache.commons.io.output.TeeOutputStream, only one copy of the string is sent to file2.

          Would you like to submit a pull request to https://github.com/jenkinsci/remoting/ with a proposed fix?

          Mark Waite added a comment - - edited Thanks for the clear description of the issue. I used Jenkins 2.414.1-rc and confirmed that the Jenkins script console shows that file2 in your example receives two copies of the testing string. When the import is replaced with org.apache.commons.io.output.TeeOutputStream , only one copy of the string is sent to file2 . Would you like to submit a pull request to https://github.com/jenkinsci/remoting/ with a proposed fix?

          jerry wiltse added a comment - - edited

          I think we can do one better. I propose that this class simply be deleted and the ONE usage of it in the remoting module be replaced with Apache commons: 

          org.apache.commons.io.output.TeeOutputStream

          The only current usage:

          https://github.com/jenkinsci/remoting/blob/e283b8ed25576ac1772cb383a58f202d9a7f72bb/src/main/java/org/jenkinsci/remoting/engine/WorkDirManager.java#L31

           

           

          The "Commons" implementation is already known to work properly, and is already used in 3 places in jenkins core: 

          https://github.com/search?q=repo%3Ajenkinsci%2Fjenkins%20TeeOutputStream&type=code

          Commons is already a dependency of remoting:

          https://github.com/jenkinsci/remoting/blob/master/pom.xml#L127-L132

          And even other mainstream plugins already use "Commons" instead of this one:

          https://github.com/jenkinsci/pipeline-utility-steps-plugin/blob/master/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/fs/TeeStep.java#L46

          I don't know what the policy is for deleting a class like this... maybe it can't be deleted for backwards compatibility, maybe it needs to be marked as deprecated for some period of time.

          In any case, if there is still any interest in the other ticket (https://issues.jenkins.io/browse/JENKINS-38520) , we can link this ticket to that one, and someone with a stake in that can see if the reproduction code here makes it easier to provide a reproduction for that issue which can prove that it's the same root cause (it seems obvious to me but probably warrants a dedicated test case).  If they do all that, they can then try switching the import to the Commons version and prove that it fixes the bug. 

          Also of note, I still don't see the error in the original code which is responsible for the problem,  otherwise I would indeed submit a PR with a fix. The code is super old and straightforward.

          jerry wiltse added a comment - - edited I think we can do one better. I propose that this class simply be deleted and the ONE usage of it in the remoting module be replaced with Apache commons:  org.apache.commons.io.output.TeeOutputStream The only current usage: https://github.com/jenkinsci/remoting/blob/e283b8ed25576ac1772cb383a58f202d9a7f72bb/src/main/java/org/jenkinsci/remoting/engine/WorkDirManager.java#L31     The "Commons" implementation is already known to work properly, and is already used in 3 places in jenkins core:  https://github.com/search?q=repo%3Ajenkinsci%2Fjenkins%20TeeOutputStream&type=code Commons is already a dependency of remoting: https://github.com/jenkinsci/remoting/blob/master/pom.xml#L127-L132 And even other mainstream plugins already use "Commons" instead of this one: https://github.com/jenkinsci/pipeline-utility-steps-plugin/blob/master/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/fs/TeeStep.java#L46 I don't know what the policy is for deleting a class like this... maybe it can't be deleted for backwards compatibility, maybe it needs to be marked as  deprecated for some period of time. In any case, if there is still any interest in the other ticket ( https://issues.jenkins.io/browse/JENKINS-38520 ) , we can link this ticket to that one, and someone with a stake in that can see if the reproduction code here makes it easier to provide a reproduction for that issue which can prove that it's the same root cause (it seems obvious to me but probably warrants a dedicated test case).  If they do all that, they can then try switching the import to the Commons version and prove that it fixes the bug.  Also of note, I still don't see the error in the original code which is responsible for the problem,  otherwise I would indeed submit a PR with a fix. The code is super old and straightforward.

          Mark Waite added a comment -

          I like that proposal very much.  Could you propose the pull request to replace the existing usage with Apache commons.io TeeOutputStream and annotate the current implementation as Deprecated?

          Mark Waite added a comment - I like that proposal very much.  Could you propose the pull request to replace the existing usage with Apache commons.io TeeOutputStream and annotate the current implementation as Deprecated?

          jerry wiltse added a comment -

          jerry wiltse added a comment - Please have a look: https://github.com/jenkinsci/remoting/pull/662

          jerry wiltse added a comment -

          Likely caused by, have not confirmed with certainty.

          jerry wiltse added a comment - Likely caused by, have not confirmed with certainty.

          jerry wiltse added a comment -

          Is likely the cause of, have not confirmed with certainty.

          jerry wiltse added a comment - Is likely the cause of, have not confirmed with certainty.

            basil Basil Crow
            solvingj jerry wiltse
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: