Stop creating unnecessary exceptions with MultiException #2580

This doesn't actually stop creating the unnecessary exception instead it
stop running the expensive `fillInStackTrace()` unless we actually need
it.

This does result in the stack trace being set to where we throw the
exception rather than when we create it.

Signed-off-by: Luke Butters <lbutters@funnelback.com>
This commit is contained in:
Luke Butters 2018-05-29 13:43:17 +10:00
parent 10070b9af6
commit 7e3242b5cc
2 changed files with 45 additions and 7 deletions

View File

@ -31,11 +31,28 @@ import java.util.List;
public class MultiException extends Exception public class MultiException extends Exception
{ {
private List<Throwable> nested; private List<Throwable> nested;
boolean superConstructorCalled = false;
/* ------------------------------------------------------------ */ /* ------------------------------------------------------------ */
public MultiException() public MultiException()
{ {
super("Multiple exceptions"); super("Multiple exceptions");
this.superConstructorCalled = true;
}
/**
* We only want to fill in the stack trace if we have an exception to throw.
*
* This prevents the stack trace from being written when the constructor is
* called, yet will let as fill it in later on e.g. when we actually want
* to throw the multi exception.
*/
@Override
public Throwable fillInStackTrace() {
if(superConstructorCalled) {
return super.fillInStackTrace();
}
return this;
} }
/* ------------------------------------------------------------ */ /* ------------------------------------------------------------ */
@ -105,6 +122,7 @@ public class MultiException extends Exception
if (th instanceof Exception) if (th instanceof Exception)
throw (Exception)th; throw (Exception)th;
default: default:
this.fillInStackTrace();
throw this; throw this;
} }
} }
@ -137,6 +155,7 @@ public class MultiException extends Exception
else else
throw new RuntimeException(th); throw new RuntimeException(th);
default: default:
this.fillInStackTrace();
throw new RuntimeException(this); throw new RuntimeException(this);
} }
} }
@ -155,7 +174,10 @@ public class MultiException extends Exception
return; return;
if (nested.size()>0) if (nested.size()>0)
{
this.fillInStackTrace();
throw this; throw this;
}
} }
/* ------------------------------------------------------------ */ /* ------------------------------------------------------------ */

View File

@ -21,10 +21,10 @@ package org.eclipse.jetty.util;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import java.io.IOException;
import org.junit.Test; import org.junit.Test;
import java.io.IOException;
public class MultiExceptionTest public class MultiExceptionTest
{ {
@ -37,6 +37,8 @@ public class MultiExceptionTest
me.ifExceptionThrow(); me.ifExceptionThrow();
me.ifExceptionThrowMulti(); me.ifExceptionThrowMulti();
me.ifExceptionThrowRuntime(); me.ifExceptionThrowRuntime();
assertEquals("Stack trace should not be filled out", 0, me.getStackTrace().length);
} }
@Test @Test
@ -91,19 +93,26 @@ public class MultiExceptionTest
{ {
assertTrue(run==e); assertTrue(run==e);
} }
assertEquals("Stack trace should not be filled out", 0, me.getStackTrace().length);
} }
@Test private MultiException multiExceptionWithTwo() {
public void testTwo() throws Exception
{
MultiException me = new MultiException(); MultiException me = new MultiException();
IOException io = new IOException("one"); IOException io = new IOException("one");
RuntimeException run = new RuntimeException("one"); RuntimeException run = new RuntimeException("one");
me.add(io); me.add(io);
me.add(run); me.add(run);
assertEquals(2,me.size()); assertEquals(2,me.size());
assertEquals("Stack trace should not be filled out", 0, me.getStackTrace().length);
return me;
}
@Test
public void testTwo() throws Exception
{
MultiException me = multiExceptionWithTwo();
try try
{ {
me.ifExceptionThrow(); me.ifExceptionThrow();
@ -112,8 +121,10 @@ public class MultiExceptionTest
catch(MultiException e) catch(MultiException e)
{ {
assertTrue(e==me); assertTrue(e==me);
assertTrue(e.getStackTrace().length > 0);
} }
me = multiExceptionWithTwo();
try try
{ {
me.ifExceptionThrowMulti(); me.ifExceptionThrowMulti();
@ -122,8 +133,10 @@ public class MultiExceptionTest
catch(MultiException e) catch(MultiException e)
{ {
assertTrue(e==me); assertTrue(e==me);
assertTrue(e.getStackTrace().length > 0);
} }
me = multiExceptionWithTwo();
try try
{ {
me.ifExceptionThrowRuntime(); me.ifExceptionThrowRuntime();
@ -132,8 +145,10 @@ public class MultiExceptionTest
catch(RuntimeException e) catch(RuntimeException e)
{ {
assertTrue(e.getCause()==me); assertTrue(e.getCause()==me);
assertTrue(e.getStackTrace().length > 0);
} }
RuntimeException run = new RuntimeException("one");
me = new MultiException(); me = new MultiException();
me.add(run); me.add(run);
me.add(run); me.add(run);
@ -146,6 +161,7 @@ public class MultiExceptionTest
catch(RuntimeException e) catch(RuntimeException e)
{ {
assertTrue(e.getCause()==me); assertTrue(e.getCause()==me);
assertTrue(e.getStackTrace().length > 0);
} }
} }