Clean Code: Refactoring eines doppelten try-Blocks

9 Kommentare

Ich habe kürzlich beim Aufräumen einer alten Codebase ein Codesnippet gefunden, das schon auf den ersten Blick verdächtig aussah. Beim Refactoring stellte sich heraus dass es nicht nur unschön war, sondern je nach Szenario auch eine Exception verschluckt werden kann. Ich erkläre in diesem Beitrag, was das Problem war und wie es behoben wurde.

Gehen wir vom eigentlichen Phänomen abstrahiert von folgendem Code aus:

public class Test {
 
    public static void main(String[] args) {
        try {
            try {
                throw new NullPointerException();
            } finally {
                throw new IllegalArgumentException();
            }
        } catch(Exception e) {
            e.printStackTrace();
        }
    }
}

Beim Betrachten des Codes kann man sich zu Recht fragen, welche Exception im catch-Block ankommt:

  • Beide Exceptions?
  • Nur die NullPointerException?
  • Nur die IllegalArgumentException?
  • Gar keine Exception?

Hier die Konsolenausgabe wenn man obigen Code ausführt:

java.lang.IllegalArgumentException
    at Test.main(Test.java:9)

Offensichtlich wird also die ursprüngliche NullPointerException „verschluckt“ und kommt niemals im catch-Block an. Das allein ist schon unschön, abgesehen davon dass zwei verschachtelte try-Blöcke sehr eigenartig wirken. Im obigen Beispiel hat man allerdings noch den Vorteil, dass Eclipse den finally-Block gelb markiert und mitteilt, dass dieser nicht normal endet.

Nun zum ursprünglichen Kontext und dem „echten“ Code-Snippet:

FileOutputStream out = null;
try {
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } finally {
        if (out != null) {
            out.close();
        }
    }
} catch (IOException e) {
    logger.error("Fehler beim Schreiben von " + filename + ": " + e.getMessage(), e);
}

Was ist schlecht an diesem Code? Erstens haben wir hier das gleiche verschachtelte try-try-Konstrukt wie oben im allgemeinen Beispiel. Dies kann also potenziell zum Verschlucken der ursprünglichen Exception führen. Außerdem ist hier das Problem einer potenziellen zusätzlichen Exception im finally-Block nicht mithilfe von Eclipse erkennbar, da FileOutputStream#close() eine IOException wirft. Da diese von dem catch-Block gefangen wird, meckert Eclipse den finally-Block nicht an. Grundsätzlich kann man über diesen Code keine Aussage treffen, ob er sich immer wie gewünscht verhält.

Um diesen Code zu verbessern, haben wir den finally-Block hinter den catch-Block verschoben und außerdem IOUtils#closeQuietly(OutputStream) verwendet, was eventuell auftretende Exceptions beim Schließen des Streams für sich behält. Das Ergebnis:

FileOutputStream out = null;
try {
    out = new FileOutputStream(filename);
    out.write(fileContent);
} catch (IOException e) {
    logger.error("Fehler beim Schreiben von " + filename + ": " + e.getMessage(), e);
} finally {
    IOUtils.closeQuietly(out);
}

Damit ist der Code nicht nur qualitativ besser, er funktioniert jetzt auch definitiv wie beabsichtigt und ist im gleichen Zuge wesentlich besser lesbar.

Eine Anmerkung zu closeQuietly: es handelt sich hierbei keineswegs um eine Patentlösung für das Schließen jeder Art von Stream. Selbstverständlich sollte man closeQuietly nur dann verwenden, wenn man genau weiß dass man die Exception auf keinen Fall sehen will. Falls man dazu aber noch etwas loggen oder sonst etwas zur Behandlung der Exception tun möchte, sollte man natürlich den klassischen Weg gehen und im finally-Block nochmal try-catch einbauen. Den Code der dabei herauskommt darf sich jeder selbst ausdenken 🙂

Kommentare

  • Peter

    20. Mai 2010 von Peter

    You definitely don’t want to ignore close() exceptions when closing output stream … and yes, they happen.

    I usually do:

    FileOutputStream out = null;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
     
        out.close();
        out = null;
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    } finally {
        IOUtils.closeQuietly(out);
    }

    This way, if close() inside try throws exception, it is thrown out, but I still quietly close stream if exception occur earlier, because there is already another exception.

    What do you think about it?

  • amrr

    20. Mai 2010 von amrr

    Robert, I found your article really interesting. I was surprised by the loss of that nullpointerexception, or at least unsure of what to expect.

    It makes me think of the possibiltiy of losing rethrown exceptions, and the importance of error-free finally blocks.

    i.e.

    try {
       xyz();
    } catch(Exception e) {
       log(e);
       throw new XxxException("Problem doing xyx();",e);
    } finally {
      bugWhichThrowsRuntimeException();
    }

    … the calling code would never see the XxxException. Maybe this is an unusual use-case, but it seems noteworthy to me.


    Peter is right: you need to call close() in the main try block, otherwise the fileContent may never be written to disk, and nothing would even be logged. I think you should add this to your code sample, then it will stand up as a really good article.

  • J.C.

    20. Mai 2010 von J.C.

    Peter definitely has the right idea. Peter’s example is the only real way to deal with close properly. Here’s the full code block. You can feel free to refactor the finally into a method if you wish.

    FileOutputStream out = null;
    try {
      out = new FileOutputStream(filename);
      out.write(fileContent);
      out.close();
      out = null;
    } finally {
      if (out != null) {
        try {
          out.close();
        } catch (IOException) {
          ExceptionHandler.handleSecondaryException("Secondary exception while writing to file: " + filename, e);
        }
        out = null;
      }
    }
  • Raja Kannappan

    21. Mai 2010 von Raja Kannappan

    Java 7 would handle this problem through automatic resource management.

    http://mail.openjdk.java.net/pipermail/coin-dev/2009-February/000011.html

  • Andrei Solntsev

    I like your example. Thanks!

  • slava

    22. Mai 2010 von slava

    So, you’ve basically changed nothing. If you’d open the IOUtils source, you’d see following:

    public static void closeQuietly(Closeable closeable) {
      try {
        if (closeable != null) {
          closeable.close();
        }
      } catch (IOException ioe) {
        // ignore
      }
    }

    Therefore your code is effectively the following:

    FileOutputStream out = null;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    } finally {
        try {
            if (out != null) {
                out.close();
            }
        } catch (IOException ioe) {}
    }

    My opinion, the original code is better as it at least logs the exception thrown while closing the stream, and your code don’t.

    The interesting for us part of the java FileOutputStream implementation is:

    private native void close0() throws IOException;

    You really cannot deal effectively with the IOException while closing your stream. It doesn’t matter, when you’ve got the exception. Your file
    is corrupted or don’t exists anyway. Why don’t you just do that:

    try {
        FileOutputStream out = new FileOutputStream(filename);
        try {
            out.write(fileContent);
        } finally {
            out.close();
        }
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    }

    Or alternatively (which i would not recommend, yes i no this is not really safe), you can let the finalize method of the FileOutputStream do the job for you.
    Then your code could be:

     try {
         FileOutputStream out = new FileOutputStream(filename);
         out.write(fileContent);
         out.close();
     } catch (IOException e) {
         logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
     }
  • Chris

    22. Mai 2010 von Chris

    Because in your code…

    try {
        FileOutputStream out = new FileOutputStream(filename);
        try {
            out.write(fileContent);
        } finally {
            out.close();
        }
    } catch (IOException e) {
        logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
    }

    if the write() throws an IOException and then in the finally block, the close() also throws an IOException your catch block will only get the IOException from the close(). The first exception is lost.
    J.C’s code is the best here as it deals with all situations.

  • Fabian Lange

    Hi Folks,
    thanks for the great discussion.
    Let’s attack this backwards.
    I looked up the actual implementation of close for File Input Stream on Windows and Solaris from the OpenJDK:
    its in io_util_md.c
    Windows:

    if (CloseHandle(h) == 0) { /* Returns zero on failure */
        SET_FD(this, fd, fid); // restore fd
        JNU_ThrowIOExceptionWithLastError(env, "close failed");
    }

    And people actually seem never to have managed to get this:
    http://www.techreplies.com/development-resources-58/can-closehandle-virtualfree-fail-381270/#

    Solaris:

    if (JVM_Close(fd) == -1) {
        SET_FD(this, fd, fid); // restore fd
        printf("JVM_Close returned -1n");
        JNU_ThrowIOExceptionWithLastError(env, "close failed");
    }

    Here the other side. Somebody reported that this actually should be handled:
    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4952229
    (note the forgotten debug output)

    So when writing to local disk this is unlikely to ever happen. OutputStream declares this, because there might be buffering implementations that need to flush() on close().

    But the Statement, that you cannot do anything is not correct. Just logging is poor mans exception handling. Logging gives no extra value. That is why Robert used IOUtils and this is perfect for that.
    You could retry or resort to a different storage in case of error.

    I actually do not see the point of coding out.close() twice, especially in regard of code complexity. It will only happen once. And it should happen always. So the correct location is the finally block. If you want to do something you might want to add a try catch finally inside the finally block.

    But this gets messy if you copy from one stream to another.

    To sum up: If you are not doing real error recovery, I consider Roberts solution to be the best.
    If you are planning for sensible error recovery you should be aware of something: the IOException can occur somewhere else. Eg from a Serialisation call. You certainly do not want to react to that one by retrying writing.

    FileOutputStream out = null;
    handleWriteError = false;
    try {
        out = new FileOutputStream(filename);
        out.write(fileContent);
    } catch (IOException e) {
        // error while writing to filename
        handleWriteError = true;
    } finally {
        try {
            out.close();
        } catch (IOException e) {
            // rare error when closing
            handleWriteError = true;
        }
    }
    if (handleWriteError) {
        File failed = new File(filename);
        if (failed.exists()) {
            if (!file.delete()) {
                sendAdminAlert("Cleanup of failed write failed. Could not remove corrupted file at "+filename);
            }
        }
        writeToSecondaryStore(filename, fileContent);
    }

    PS: One should actually really consider using BufferedXyz for performance reasons.

  • Fabian Lange

    PPS:
    My favorite Solution is: rethrow the Exception
    For many people this is not that acceptable, but it is often easier than to write good Exception Handling in the method causing it.

Kommentieren

Deine E-Mail-Adresse wird nicht veröffentlicht. Erforderliche Felder sind mit * markiert.