CODEWITHSUNDEEP
javaexception
Joshua MN
Joshua MN

Reputation: 1516

Is it safe to store an instance of an Exception and reuse it?

Is it safe to:

public class Widget {

    private static final IllegalStateException LE_EXCEPTION
            = new IllegalStateException("le sophisticated way");

    ...

   public void fun() {
      // some logic here, that may throw
      throw LE_EXCEPTION;
   }

   ....
}
  1. keep an instance of an exception
  2. use it whenever needed (throw)

instead of throwing a new exception each time?

I am interested if it's safe

By safe I mean: no memory corruption, no additional exceptions thrown by JVM, by missing classes, no bad classes loaded (...). Note: the exception will be thrown over network (remoting).

Other issues (readability, cost of keeping an instance) are not important.

Upvotes: 21

Views: 2784

Answers (6)

user207421
user207421

Reputation: 311023

It isn't safe to get fancy with any error-handling code. Keep it simple, keep it obvious, and don't write any thing you need to ask questions about. Error handling is no place to be incurring extra errors.

Upvotes: 2

irreputable
irreputable

Reputation: 45453

It is not safe, unless the exception is immutable (i.e. enableSuppression=writableStackTrace=false).

If an exception is not immutable, it can be modified by a catcher - setting a new stacktrace or adding a suppressed exception. If there are multiple catchers trying to modify the exception, there'll be chaos.

Astonishingly, Throwable is actually thread-safe, for god-knows-what. So at least there won't be catastrophic failure if an exception is modified by multiple threads. But there will be logic failure.

Memory leak is also possible, if app keeps adding suppressed exceptions to this long-live exception.

Upvotes: 8

T.J. Crowder
T.J. Crowder

Reputation: 1075309

It depends on your definition of "safe." The exception will give a misleading stack trace, which I wouldn't call "safe". Consider:

public class ThrowTest {
    private static Exception e = new Exception("t1"); // Line 2

    public static final void main(String[] args) {
        ThrowTest tt;

        tt = new ThrowTest();
        try {
            tt.t1();
        }
        catch (Exception ex) {
            System.out.println("t1:");
            ex.printStackTrace(System.out);
        }
        try {
            tt.t2();                                  // Line 16
        }
        catch (Exception ex) {
            System.out.println("t2:");
            ex.printStackTrace(System.out);
        }
    }

    private void t1() 
    throws Exception {
        throw this.e;
    }

    private void t2() 
    throws Exception {
        throw new Exception("t2");                    // Line 31
    }
}

That has this output:

$ java ThrowTest
t1:
java.lang.Exception: t1
    at ThrowTest.<clinit>(ThrowTest.java:2)
t2:
java.lang.Exception: t2
    at ThrowTest.t2(ThrowTest.java:31)
    at ThrowTest.main(ThrowTest.java:16)

Note how the t1 method is completely missing from the stack trace in the first test case. There's no useful context information at all.

Now, you can use fillInStackTrace to fill in that information just before the throw:

this.e.fillInStackTrace();
throw this.e;

...but that's just making work for yourself (work you will forget sometimes). There's simply no benefit to it at all. And not all exceptions allow you to do it (some exceptions make the stack trace read-only).

You've said elsewhere in the comments that this is to avoid "code duplication." You're much better off having an exception builder function:

private IllegalStateException buildISE() {
    return new IllegalStateException("le sophisticated way");
}

(Could be static final if you like.)

And then throwing it like this:

throw buildISE();

That avoids code duplication without misleading stack traces and unnecessary Exception instances.

Here's what that looks like applied to the above:

public class ThrowTest {

    public static final void main(String[] args) {
        ThrowTest tt;

        tt = new ThrowTest();
        try {
            tt.t1();                                   // Line 8
        }
        catch (Exception ex) {
            System.out.println("t1:");
            ex.printStackTrace(System.out);
        }
        try {
            tt.t2();                                   // Line 15
        }
        catch (Exception ex) {
            System.out.println("t2:");
            ex.printStackTrace(System.out);
        }
    }

    private static final Exception buildEx() {
        return new Exception("t1");                    // Line 24
    }

    private void t1() 
    throws Exception {
        throw buildEx();                               // Line 29
    }

    private void t2() 
    throws Exception {
        throw new Exception("t2");                     // Line 34
    }
}

$ java ThrowTest
t1:
java.lang.Exception: t1
    at ThrowTest.buildEx(ThrowTest.java:24)
    at ThrowTest.t1(ThrowTest.java:29)
    at ThrowTest.main(ThrowTest.java:8)
t2:
java.lang.Exception: t2
    at ThrowTest.t2(ThrowTest.java:34)
    at ThrowTest.main(ThrowTest.java:15)

Upvotes: 20

AlexR
AlexR

Reputation: 115378

It think it can cause problems when reading the exception's stack trace. The stack trace is filled when code throws exception but is stored into the instance of the exception itself. If you throw the same instance of exception twice I think that getStackTrace() returns the last stack trace. If for some reason (e.g. in multithreaded environment) exception is thrown twice from different places in code and then is printed the stack trace from first throwing may be wrong.

Since there is no reason to re-use instances of the exception I do not recommend you to do so.

If you want to use exception as a kind of container that holds additional information (e.g. error message, error code etc) use the fact that exception is serializable: clone it before throwing. So, every time you will throw unique instance of the exception but its fields will be copied from pre-created template.

Upvotes: 2

Nikita Tkachenko
Nikita Tkachenko

Reputation: 2146

One more reason not to do it is that the stack trace will be inappropriate.

No matter in what place in your code you throw the exception, when its stack trace is printed, it'll show the line where the exception is initialized instead of the line where it is being thrown (in this particular case instead of expected Widget.fun() the stack trace will contain Widget.<clinit>).

So you (or whoever uses your code) won't be able to determine where the error actually lies.

Upvotes: 3

comanitza
comanitza

Reputation: 1103

I think this is wrong, because it will encourage you to use use exceptions to describe normal situations, and no just exceptional ones. See Effective Java second edition by J. Bloch, item 57, page 241.

Also you are always keeping an object in the heap, so this is not necessary, because object creation is very fast in moder JVMs and also once an exception is thrown it will be probably very quickly garbage collected.

Also your code could become very misleading, this adding a lot of overhead for something pretty strait forward.

Upvotes: 1

Related Questions