8
\$\begingroup\$

I am just sharing the method that i am using to convert stacktrace to string. This stacktrace is then being used in AWS Lambda to log. There are two things i am mainly concerned about

private String convertStackTrace(Throwable throwable){
    StringWriter stringWriter = new StringWriter();
    PrintWriter printWriter = new PrintWriter(stringWriter);
    String stackTrace;

    try {
        throwable.printStackTrace(printWriter);
        stackTrace = stringWriter.toString();
    }
    catch(Exception e){
        logSelf("Error converting exception. Simple exception message will be logged");
        stackTrace = throwable.getMessage();
    } finally {
        try {
            stringWriter.flush();
            stringWriter.close();
            printWriter.flush();
            printWriter.close();
        } catch (Exception e){
            logSelf("Error closing writers");
        }
    }

    return stackTrace;
}

Here is logSelf method

private void logSelf(String error){
    lambdaLogger.log(formatMessage(
            this.getClass().getCanonicalName(), LOG_LEVEL_ERROR, error, null)
    );
}
  1. Shall i be opening/closing those printwriters everytime an error is logged?
  2. Is it correct way to convert stacktrace to string?
\$\endgroup\$

1 Answer 1

Reset to default
5
\$\begingroup\$

Yes, you should be opening/closing the printwriters each time.

Yes, conceptually it's a decent way to convert the stack trace..... but... there are concerns, and is there a better way?

The most significant concern is that, if there's a problem writing to the PrintWriter, the call throwable.printStackTrace(printWriter); will fail, and the method will return a default value. This is not ideal.

The reality, though, is that those methods can never fail because the IO is to a StringWriter, and there's no actual IO component. A failure there would be..... inconceivable. None of your exception handling can really happen... it's just not going to fail (the "Titanic Logger").

The issue is that your code is checking for impossible conditions in a way that's overkill.

Still, using some Java 7 semantics, you can use try-with-resource functionality, and get a much more concise method:

private static String convertStackTrace(Throwable throwable) {
    try (StringWriter sw = new StringWriter();
             PrintWriter pw = new PrintWriter(sw)) {

        throwable.printStackTrace(pw);

        return sw.toString();

    } catch (IOException ioe) {
        // can never really happen..... convert to unchecked exception
        throw new IllegalStateException(ioe);
    }
}

Note that the method is now static, and it does not need the finally section to clean up.

You can see this running in ideone: https://ideone.com/rKj9mT

\$\endgroup\$
4
  • \$\begingroup\$ Thanks. But you aren't closing PrintWriter and StringWriter. Does it matter here? I would go with approach of not using try/catch block \$\endgroup\$ Commented Jun 6, 2017 at 20:37
  • \$\begingroup\$ @EmAe - I am closing them.... that's what the try (... ) { ... } with resources does: docs.oracle.com/javase/tutorial/essential/exceptions/… \$\endgroup\$ Commented Jun 6, 2017 at 21:00
  • \$\begingroup\$ Oh, I see. So what if i don't add try/catch block. Since they aren't adding any value. Shall i close them explicitly? \$\endgroup\$ Commented Jun 6, 2017 at 21:08
  • \$\begingroup\$ Since the throwable.printStackTrace( ) throws an exception you have to catch it anyway. Since the introduction of the try with recourses in java 7, this construct is the preferred way. Although I would just leave the catch block empty except for the comment that it cannot happen in this case. \$\endgroup\$ Commented Jun 7, 2017 at 7:00

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.