Hadoop Common
  1. Hadoop Common
  2. HADOOP-3073

SocketOutputStream.close() should close the channel.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.17.0
    • Fix Version/s: 0.17.0
    • Component/s: ipc
    • Labels:
      None
    • Release Note:
      SocketOutputStream.close() closes the underlying channel. Increase compatibility with java.net.Socket.getOutputStream. User Impact : none.
    1. HADOOP-3073-javadoc.patch
      1 kB
      Raghu Angadi
    2. HADOOP-3073.patch
      4 kB
      Raghu Angadi

      Activity

      Hide
      Raghu Angadi added a comment -

      Now I see the source of confusion. I have search shortcut for javadoc to go to Java SE 5.0. SE 6 explicitly states that closing this stream closes the socket, but SE 5.0 doesn't.

      Show
      Raghu Angadi added a comment - Now I see the source of confusion. I have search shortcut for javadoc to go to Java SE 5.0. SE 6 explicitly states that closing this stream closes the socket, but SE 5.0 doesn't.
      Hide
      Raghu Angadi added a comment -

      I think this is blocker for 0.17. The fix is also safe and simple.

      Show
      Raghu Angadi added a comment - I think this is blocker for 0.17. The fix is also safe and simple.
      Hide
      Raghu Angadi added a comment -

      Patch attached. SocketInputStream and SocketOutputStream close the channel when close() is invoked by the user.

      Also these two classes have getChannel() call so that underlying channel could be used for something thing like FileChannel.transferTo().

      Show
      Raghu Angadi added a comment - Patch attached. SocketInputStream and SocketOutputStream close the channel when close() is invoked by the user. Also these two classes have getChannel() call so that underlying channel could be used for something thing like FileChannel.transferTo().
      Hide
      Tsz Wo Nicholas Sze added a comment -

      Do we need try-finally in close()? i.e.

      try{
        reader.channel.close();
      } finally {
        reader.close();
      }
      
      Show
      Tsz Wo Nicholas Sze added a comment - Do we need try-finally in close()? i.e. try { reader.channel.close(); } finally { reader.close(); }
      Hide
      Raghu Angadi added a comment -

      I don't think so. The fact that close() throws exception implies to user something is wrong and its ok for something to be wrong at that time.

      Show
      Raghu Angadi added a comment - I don't think so. The fact that close() throws exception implies to user something is wrong and its ok for something to be wrong at that time.
      Hide
      Raghu Angadi added a comment -

      > something to be wrong at that time.
      I meant its ok for 'close' flag not to be set.

      Show
      Raghu Angadi added a comment - > something to be wrong at that time. I meant its ok for 'close' flag not to be set.
      Hide
      Tsz Wo Nicholas Sze added a comment -

      +1 the patch looks good

      Show
      Tsz Wo Nicholas Sze added a comment - +1 the patch looks good
      Hide
      Raghu Angadi added a comment -

      Thanks Nicholas. This a blocker for 0.17.

      Show
      Raghu Angadi added a comment - Thanks Nicholas. This a blocker for 0.17.
      Hide
      Hadoop QA added a comment -

      -1 overall. Here are the results of testing the latest attachment
      http://issues.apache.org/jira/secure/attachment/12378602/HADOOP-3073.patch
      against trunk revision 619744.

      @author +1. The patch does not contain any @author tags.

      tests included +1. The patch appears to include 3 new or modified tests.

      javadoc -1. The javadoc tool appears to have generated 1 warning messages.

      javac +1. The applied patch does not generate any new javac compiler warnings.

      release audit +1. The applied patch does not generate any new release audit warnings.

      findbugs -1. The patch appears to introduce 1 new Findbugs warnings.

      core tests +1. The patch passed core unit tests.

      contrib tests +1. The patch passed contrib unit tests.

      Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/testReport/
      Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/artifact/trunk/build/test/checkstyle-errors.html
      Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/console

      This message is automatically generated.

      Show
      Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12378602/HADOOP-3073.patch against trunk revision 619744. @author +1. The patch does not contain any @author tags. tests included +1. The patch appears to include 3 new or modified tests. javadoc -1. The javadoc tool appears to have generated 1 warning messages. javac +1. The applied patch does not generate any new javac compiler warnings. release audit +1. The applied patch does not generate any new release audit warnings. findbugs -1. The patch appears to introduce 1 new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2068/console This message is automatically generated.
      Hide
      Raghu Angadi added a comment -

      I think this findbugs warning need to be waived. close() removed below was added to fix the same findbugs warning in HADOOP-2346 as described in a comment.

      affected code:

      -      try {
      -        //write the header.
      -        out.writeShort( DATA_TRANSFER_VERSION );
      -        out.write( OP_READ_BLOCK );
      -        out.writeLong( blockId );
      -        out.writeLong( startOffset );
      -        out.writeLong( len );
      -        out.flush();
      -      } finally {
      -        IOUtils.closeStream(out);
      -      }
      +      //write the header.
      +      out.writeShort( DATA_TRANSFER_VERSION );
      +      out.write( OP_READ_BLOCK );
      +      out.writeLong( blockId );
      +      out.writeLong( startOffset );
      +      out.writeLong( len );
      +      out.flush();
      
      Show
      Raghu Angadi added a comment - I think this findbugs warning need to be waived. close() removed below was added to fix the same findbugs warning in HADOOP-2346 as described in a comment . affected code: - try { - //write the header. - out.writeShort( DATA_TRANSFER_VERSION ); - out.write( OP_READ_BLOCK ); - out.writeLong( blockId ); - out.writeLong( startOffset ); - out.writeLong( len ); - out.flush(); - } finally { - IOUtils.closeStream(out); - } + //write the header. + out.writeShort( DATA_TRANSFER_VERSION ); + out.write( OP_READ_BLOCK ); + out.writeLong( blockId ); + out.writeLong( startOffset ); + out.writeLong( len ); + out.flush();
      Hide
      Raghu Angadi added a comment -

      I just committed this. - Only way I can think of avoiding findbugs warning is to write a FilterSocket class.

      Show
      Raghu Angadi added a comment - I just committed this. - Only way I can think of avoiding findbugs warning is to write a FilterSocket class.
      Hide
      Hudson added a comment -
      Show
      Hudson added a comment - Integrated in Hadoop-trunk #443 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/443/ )
      Hide
      Raghu Angadi added a comment -

      I committed this with javadoc errors by mistake. will commit attached javadoc.patch to fix that.

      Show
      Raghu Angadi added a comment - I committed this with javadoc errors by mistake. will commit attached javadoc.patch to fix that.
      Hide
      Hudson added a comment -
      Show
      Hudson added a comment - Integrated in Hadoop-trunk #444 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/444/ )

        People

        • Assignee:
          Raghu Angadi
          Reporter:
          Raghu Angadi
        • Votes:
          0 Vote for this issue
          Watchers:
          1 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development