Uploaded image for project: 'Hive'
  1. Hive
  2. HIVE-7929

close of ZipOutputStream in Utils#jarDir() should be placed in finally block

    Details

    • Type: Bug
    • Status: Patch Available
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.13.1
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      Description

      In accumulo-handler/src/java/org/apache/hadoop/hive/accumulo/Utils.java , line 308 :

      zos.closeEntry();
      zipDir(dir, relativePath, zos, true);
      zos.close();

      If exception is happened, ZipOutputStream would be left unclosed..

      1. HIVE-7929_002.patch
        2 kB
        skrho
      2. HIVE-7929_001.patch
        2 kB
        skrho

        Activity

        Hide
        rsk13th skrho added a comment -

        zos.closeEntry() is called twice if manifestFile is not existed. So I moved zos.closeEntry() to last try statement..

        Please review my patch 2 ^^ ..

        Show
        rsk13th skrho added a comment - zos.closeEntry() is called twice if manifestFile is not existed. So I moved zos.closeEntry() to last try statement.. Please review my patch 2 ^^ ..
        Hide
        elserj Josh Elser added a comment -

        The catch block is unnecessary. I think the finally block should only contain zos.close() with zos.closeEntry(); and zipDir(dir, relativePath, zos, true); moved inside of the try block. For example:

        try {
         ...
          zos.closeEntry();
          zipDir(dir, relativePath, zos, true);
        } finally {
          zos.close();
        }
        

        Alternatively, it might be cleaner to do a try/finally in createJar(File, File) to close the JarOutputStream and completely remove the close() call in jarDir(File, String, ZipOutputStream).

        Also, it may interest you, this code was borrowed from HBase. They may benefit from these same improvements in their codebase – I forget what HBase version I copied this from though.

        Show
        elserj Josh Elser added a comment - The catch block is unnecessary. I think the finally block should only contain zos.close() with zos.closeEntry(); and zipDir(dir, relativePath, zos, true); moved inside of the try block. For example: try { ... zos.closeEntry(); zipDir(dir, relativePath, zos, true ); } finally { zos.close(); } Alternatively, it might be cleaner to do a try/finally in createJar(File, File) to close the JarOutputStream and completely remove the close() call in jarDir(File, String, ZipOutputStream) . Also, it may interest you, this code was borrowed from HBase. They may benefit from these same improvements in their codebase – I forget what HBase version I copied this from though.
        Hide
        skrho skrho added a comment -

        Here is patch..
        I added try/catch/finally statement.. and IZipOutputStream object is closed in finally statement..

        Please review my source code.. ^^ Thank you..

        Show
        skrho skrho added a comment - Here is patch.. I added try/catch/finally statement.. and IZipOutputStream object is closed in finally statement.. Please review my source code.. ^^ Thank you..

          People

          • Assignee:
            rsk13th skrho
            Reporter:
            skrho skrho
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development