From 0e2fca849257e2b6142d0aa71d76ce17e0abcd49 Mon Sep 17 00:00:00 2001 From: Matt Jacobs Date: Thu, 19 Feb 2015 15:57:22 -0800 Subject: [PATCH 1/2] Revert "Removed ExceptionThreadingUtility. Stack traces are not helpful in an event-loop world." This reverts commit ac92b6a7a5347495b63a1b76f244541b2858b6a8. --- .../exception/HystrixRuntimeException.java | 3 + .../util/ExceptionThreadingUtility.java | 120 ++++++++++++++++++ .../util/ExceptionThreadingUtilityTest.java | 52 ++++++++ 3 files changed, 175 insertions(+) create mode 100644 hystrix-core/src/main/java/com/netflix/hystrix/util/ExceptionThreadingUtility.java create mode 100644 hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java diff --git a/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java b/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java index 17feccbdb..b70a877f2 100644 --- a/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java +++ b/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java @@ -17,6 +17,7 @@ import com.netflix.hystrix.HystrixCommand; import com.netflix.hystrix.HystrixInvokable; +import com.netflix.hystrix.util.ExceptionThreadingUtility; /** * RuntimeException that is thrown when a {@link HystrixCommand} fails and does not have a fallback. @@ -39,6 +40,7 @@ public HystrixRuntimeException(FailureType failureCause, Class commandClass, String message, Throwable cause, Throwable fallbackException) { @@ -46,6 +48,7 @@ public HystrixRuntimeException(FailureType failureCause, Class + * This is used to make the exceptions thrown from an isolation thread useful, otherwise they see a stacktrace on a thread but never see who was calling it. + */ +public class ExceptionThreadingUtility { + + private final static String messageForCause = "Calling Thread included as the last 'caused by' on the chain."; + + /* package */ static void attachCallingThreadStack(Throwable e, StackTraceElement[] stack) { + Set seenCauses = new HashSet(); + + Throwable callingThrowable = new Throwable(messageForCause); + if (stack[0].toString().startsWith("java.lang.Thread.getStackTrace")) { + // get rid of the first item on the stack that says "java.lang.Thread.getStackTrace" + StackTraceElement newStack[] = Arrays.copyOfRange(stack, 1, stack.length); + stack = newStack; + } + callingThrowable.setStackTrace(stack); + + while (e.getCause() != null) { + e = e.getCause(); + if (seenCauses.contains(e)) { + break; + } else { + seenCauses.add(e); + } + } + // check that we're not recursively wrapping an exception that already had the cause set, and if not then add our artificial 'cause' + if (!messageForCause.equals(e.getMessage())) { + // we now have 'e' as the last in the chain + try { + e.initCause(callingThrowable); + } catch (Throwable t) { + // ignore + // the javadocs say that some Throwables (depending on how they're made) will never + // let me call initCause without blowing up even if it returns null + } + } + } + + @SuppressWarnings("unused") + private static String getStackTraceAsString(StackTraceElement[] stack) { + StringBuilder s = new StringBuilder(); + boolean firstLine = true; + for (StackTraceElement e : stack) { + if (e.toString().startsWith("java.lang.Thread.getStackTrace")) { + // we'll ignore this one + continue; + } + if (!firstLine) { + s.append("\n\t"); + } + s.append(e.toString()); + firstLine = false; + } + return s.toString(); + } + + public static void attachCallingThreadStack(Throwable e) { + try { + if (callingThreadCache.get() != null) { + /** + * benjchristensen -> I don't like this code, but it's been working in API prod for several months (as of Jan 2012) to satisfy debugability requirements. + * + * It works well when someone does Command.execute() since the calling threads blocks so its stacktrace shows exactly what is calling the command. + * + * It won't necessarily work very well with Command.queue() since the calling thread will be off doing something else when this occurs. + * + * I'm also somewhat concerned about the thread-safety of this. I have no idea what the guarantees are, but we haven't seen issues such as deadlocks, infinite loops, etc and no known + * data corruption. + * + * I'm not a fan of this solution, but it is better so far than not having it. + * + * The cleaner alternative would be to capture the stackTrace when the command is queued, but I'm concerned with the performance implications of capturing the stackTrace on every + * single invocation when we only need it in error events which are rare. I have not performed any testing to determine the cost, but it certainly has a cost based on looking at the + * java source code for getStackTrace(). + */ + attachCallingThreadStack(e, callingThreadCache.get().getStackTrace()); + } + } catch (Throwable ex) { + // ignore ... we don't want to blow up while trying to augment the stacktrace on a real exception + } + } + + /** + * Allow a parent thread to pass its reference in on a child thread and be remembered so that stacktraces can include the context of who called it. + *

+ * ThreadVariable is a Netflix version of ThreadLocal that takes care of cleanup as part of the request/response loop. + */ + private static ThreadLocal callingThreadCache = new ThreadLocal(); + + // TODO this doesn't get cleaned up after moving away from the Netflix ThreadVariable + + public static void assignCallingThread(Thread callingThread) { + callingThreadCache.set(callingThread); + } + +} diff --git a/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java b/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java new file mode 100644 index 000000000..a59bfc7a9 --- /dev/null +++ b/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java @@ -0,0 +1,52 @@ +package com.netflix.hystrix.util; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class ExceptionThreadingUtilityTest { + private final Throwable ex1 = new Throwable("Ex1"); + private final Throwable ex2 = new Throwable("Ex2", ex1); + + public ExceptionThreadingUtilityTest() { + ex1.initCause(ex2); + } + + @Test + public void testAttachCallingThreadStackParentThenChild() { + ExceptionThreadingUtility.attachCallingThreadStack(ex1, ex2.getStackTrace()); + assertEquals("Ex1", ex1.getMessage()); + assertEquals("Ex2", ex1.getCause().getMessage()); + assertEquals("Ex2", ex2.getMessage()); + assertEquals("Ex1", ex2.getCause().getMessage()); + } + + @Test + public void testAttachCallingThreadStackChildThenParent() { + ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex1.getStackTrace()); + assertEquals("Ex1", ex1.getMessage()); + assertEquals("Ex2", ex1.getCause().getMessage()); + assertEquals("Ex2", ex2.getMessage()); + assertEquals("Ex1", ex2.getCause().getMessage()); + } + + @Test + public void testAttachCallingThreadStackAddExceptionsToEachOther() { + ExceptionThreadingUtility.attachCallingThreadStack(ex1, ex2.getStackTrace()); + ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex1.getStackTrace()); + assertEquals("Ex1", ex1.getMessage()); + assertEquals("Ex2", ex2.getMessage()); + assertEquals("Ex2", ex1.getCause().getMessage()); + assertEquals("Ex1", ex2.getCause().getMessage()); + } + + @Test + public void testAttachCallingThreadStackAddExceptionToItself() { + ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex2.getStackTrace()); + assertEquals("Ex1", ex1.getMessage()); + assertEquals("Ex2", ex1.getCause().getMessage()); + assertEquals("Ex2", ex2.getMessage()); + assertEquals("Ex1", ex2.getCause().getMessage()); + } + +} From 1f60aaec658bf4d1814384a25c9f7b6670d69d8d Mon Sep 17 00:00:00 2001 From: Matt Jacobs Date: Thu, 19 Feb 2015 15:59:57 -0800 Subject: [PATCH 2/2] Instead of outright removing ExceptionThreadingUtility, just deprecate the public methods for backwards-compatibility's sake --- .../exception/HystrixRuntimeException.java | 2 - .../util/ExceptionThreadingUtility.java | 100 +----------------- .../util/ExceptionThreadingUtilityTest.java | 52 --------- 3 files changed, 5 insertions(+), 149 deletions(-) delete mode 100644 hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java diff --git a/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java b/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java index b70a877f2..5d136584f 100644 --- a/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java +++ b/hystrix-core/src/main/java/com/netflix/hystrix/exception/HystrixRuntimeException.java @@ -40,7 +40,6 @@ public HystrixRuntimeException(FailureType failureCause, Class commandClass, String message, Throwable cause, Throwable fallbackException) { @@ -48,7 +47,6 @@ public HystrixRuntimeException(FailureType failureCause, Class - * This is used to make the exceptions thrown from an isolation thread useful, otherwise they see a stacktrace on a thread but never see who was calling it. - */ +@Deprecated public class ExceptionThreadingUtility { - private final static String messageForCause = "Calling Thread included as the last 'caused by' on the chain."; - - /* package */ static void attachCallingThreadStack(Throwable e, StackTraceElement[] stack) { - Set seenCauses = new HashSet(); - - Throwable callingThrowable = new Throwable(messageForCause); - if (stack[0].toString().startsWith("java.lang.Thread.getStackTrace")) { - // get rid of the first item on the stack that says "java.lang.Thread.getStackTrace" - StackTraceElement newStack[] = Arrays.copyOfRange(stack, 1, stack.length); - stack = newStack; - } - callingThrowable.setStackTrace(stack); - - while (e.getCause() != null) { - e = e.getCause(); - if (seenCauses.contains(e)) { - break; - } else { - seenCauses.add(e); - } - } - // check that we're not recursively wrapping an exception that already had the cause set, and if not then add our artificial 'cause' - if (!messageForCause.equals(e.getMessage())) { - // we now have 'e' as the last in the chain - try { - e.initCause(callingThrowable); - } catch (Throwable t) { - // ignore - // the javadocs say that some Throwables (depending on how they're made) will never - // let me call initCause without blowing up even if it returns null - } - } - } - - @SuppressWarnings("unused") - private static String getStackTraceAsString(StackTraceElement[] stack) { - StringBuilder s = new StringBuilder(); - boolean firstLine = true; - for (StackTraceElement e : stack) { - if (e.toString().startsWith("java.lang.Thread.getStackTrace")) { - // we'll ignore this one - continue; - } - if (!firstLine) { - s.append("\n\t"); - } - s.append(e.toString()); - firstLine = false; - } - return s.toString(); - } - + @Deprecated //this functionality is no longer supported public static void attachCallingThreadStack(Throwable e) { - try { - if (callingThreadCache.get() != null) { - /** - * benjchristensen -> I don't like this code, but it's been working in API prod for several months (as of Jan 2012) to satisfy debugability requirements. - * - * It works well when someone does Command.execute() since the calling threads blocks so its stacktrace shows exactly what is calling the command. - * - * It won't necessarily work very well with Command.queue() since the calling thread will be off doing something else when this occurs. - * - * I'm also somewhat concerned about the thread-safety of this. I have no idea what the guarantees are, but we haven't seen issues such as deadlocks, infinite loops, etc and no known - * data corruption. - * - * I'm not a fan of this solution, but it is better so far than not having it. - * - * The cleaner alternative would be to capture the stackTrace when the command is queued, but I'm concerned with the performance implications of capturing the stackTrace on every - * single invocation when we only need it in error events which are rare. I have not performed any testing to determine the cost, but it certainly has a cost based on looking at the - * java source code for getStackTrace(). - */ - attachCallingThreadStack(e, callingThreadCache.get().getStackTrace()); - } - } catch (Throwable ex) { - // ignore ... we don't want to blow up while trying to augment the stacktrace on a real exception - } + //no-op now } - /** - * Allow a parent thread to pass its reference in on a child thread and be remembered so that stacktraces can include the context of who called it. - *

- * ThreadVariable is a Netflix version of ThreadLocal that takes care of cleanup as part of the request/response loop. - */ - private static ThreadLocal callingThreadCache = new ThreadLocal(); - - // TODO this doesn't get cleaned up after moving away from the Netflix ThreadVariable - + @Deprecated //this functionality is no longer supported public static void assignCallingThread(Thread callingThread) { - callingThreadCache.set(callingThread); + //no-op now } - } diff --git a/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java b/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java deleted file mode 100644 index a59bfc7a9..000000000 --- a/hystrix-core/src/test/java/com/netflix/hystrix/util/ExceptionThreadingUtilityTest.java +++ /dev/null @@ -1,52 +0,0 @@ -package com.netflix.hystrix.util; - -import static org.junit.Assert.assertEquals; - -import org.junit.Test; - -public class ExceptionThreadingUtilityTest { - private final Throwable ex1 = new Throwable("Ex1"); - private final Throwable ex2 = new Throwable("Ex2", ex1); - - public ExceptionThreadingUtilityTest() { - ex1.initCause(ex2); - } - - @Test - public void testAttachCallingThreadStackParentThenChild() { - ExceptionThreadingUtility.attachCallingThreadStack(ex1, ex2.getStackTrace()); - assertEquals("Ex1", ex1.getMessage()); - assertEquals("Ex2", ex1.getCause().getMessage()); - assertEquals("Ex2", ex2.getMessage()); - assertEquals("Ex1", ex2.getCause().getMessage()); - } - - @Test - public void testAttachCallingThreadStackChildThenParent() { - ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex1.getStackTrace()); - assertEquals("Ex1", ex1.getMessage()); - assertEquals("Ex2", ex1.getCause().getMessage()); - assertEquals("Ex2", ex2.getMessage()); - assertEquals("Ex1", ex2.getCause().getMessage()); - } - - @Test - public void testAttachCallingThreadStackAddExceptionsToEachOther() { - ExceptionThreadingUtility.attachCallingThreadStack(ex1, ex2.getStackTrace()); - ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex1.getStackTrace()); - assertEquals("Ex1", ex1.getMessage()); - assertEquals("Ex2", ex2.getMessage()); - assertEquals("Ex2", ex1.getCause().getMessage()); - assertEquals("Ex1", ex2.getCause().getMessage()); - } - - @Test - public void testAttachCallingThreadStackAddExceptionToItself() { - ExceptionThreadingUtility.attachCallingThreadStack(ex2, ex2.getStackTrace()); - assertEquals("Ex1", ex1.getMessage()); - assertEquals("Ex2", ex1.getCause().getMessage()); - assertEquals("Ex2", ex2.getMessage()); - assertEquals("Ex1", ex2.getCause().getMessage()); - } - -}