Hive
  1. Hive
  2. HIVE-7929

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

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Minor 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_001.patch
        2 kB
        skrho
      2. HIVE-7929_002.patch
        2 kB
        skrho

        Activity

        Hide
        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 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..
        Hide
        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
        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 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
        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 ^^ ..

          People

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

            Dates

            • Created:
              Updated:

              Development