From 2d2510b009efc9273c9eb5f2fe9e998203433ea6 Mon Sep 17 00:00:00 2001 From: Matt Nelson Date: Thu, 11 Jun 2015 16:55:48 -0500 Subject: [PATCH 1/1] HTRACE-178. Do not double wrap runnable/callable --- ...178.-Do-not-double-wrap-runnable-callable.patch | 232 +++++++++++++++++++++ htrace-core/pom.xml | 5 + .../htrace/wrappers/TraceExecutorService.java | 26 ++- .../htrace/impl/TestLocalFileSpanReceiver.java | 2 - .../htrace/wrappers/TestTraceExecutorService.java | 99 +++++++++ pom.xml | 6 + 6 files changed, 363 insertions(+), 7 deletions(-) create mode 100644 0001-HTRACE-178.-Do-not-double-wrap-runnable-callable.patch create mode 100644 htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java diff --git a/0001-HTRACE-178.-Do-not-double-wrap-runnable-callable.patch b/0001-HTRACE-178.-Do-not-double-wrap-runnable-callable.patch new file mode 100644 index 0000000..0c0262f --- /dev/null +++ b/0001-HTRACE-178.-Do-not-double-wrap-runnable-callable.patch @@ -0,0 +1,232 @@ +From ba26389a57dd44539b8d04e5284b703bd5d574db Mon Sep 17 00:00:00 2001 +From: Matt Nelson +Date: Thu, 11 Jun 2015 16:53:02 -0500 +Subject: [PATCH 1/1] HTRACE-178. Do not double wrap runnable/callable + +--- + htrace-core/pom.xml | 5 + + .../htrace/wrappers/TraceExecutorService.java | 26 +++++- + .../htrace/impl/TestLocalFileSpanReceiver.java | 2 - + .../htrace/wrappers/TestTraceExecutorService.java | 101 +++++++++++++++++++++ + pom.xml | 6 ++ + 5 files changed, 133 insertions(+), 7 deletions(-) + create mode 100644 htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java + +diff --git a/htrace-core/pom.xml b/htrace-core/pom.xml +index 6026510..5c4715d 100644 +--- a/htrace-core/pom.xml ++++ b/htrace-core/pom.xml +@@ -104,6 +104,11 @@ language governing permissions and limitations under the License. --> + commons-logging + commons-logging + ++ ++ org.mockito ++ mockito-core ++ test ++ + + + +diff --git a/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java b/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java +index 03e891f..5776354 100644 +--- a/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java ++++ b/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java +@@ -37,7 +37,7 @@ public class TraceExecutorService implements ExecutorService { + + @Override + public void execute(Runnable command) { +- impl.execute(new TraceRunnable(command)); ++ impl.execute(wrap(command)); + } + + @Override +@@ -68,24 +68,40 @@ public class TraceExecutorService implements ExecutorService { + + @Override + public Future submit(Callable task) { +- return impl.submit(new TraceCallable(task)); ++ return impl.submit(wrap(task)); + } + + @Override + public Future submit(Runnable task, T result) { +- return impl.submit(new TraceRunnable(task), result); ++ return impl.submit(wrap(task), result); + } + + @Override + public Future submit(Runnable task) { +- return impl.submit(new TraceRunnable(task)); ++ return impl.submit(wrap(task)); ++ } ++ ++ static Runnable wrap(Runnable task) { ++ if (task instanceof TraceRunnable) { ++ return task; ++ } ++ ++ return new TraceRunnable(task); ++ } ++ ++ static Callable wrap(Callable task) { ++ if (task instanceof TraceCallable) { ++ return task; ++ } ++ ++ return new TraceCallable(task); + } + + private Collection> wrapCollection( + Collection> tasks) { + List> result = new ArrayList>(); + for (Callable task : tasks) { +- result.add(new TraceCallable(task)); ++ result.add(wrap(task)); + } + return result; + } +diff --git a/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java b/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java +index 634bef8..219e97a 100644 +--- a/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java ++++ b/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java +@@ -22,12 +22,10 @@ import java.io.IOException; + import java.util.HashMap; + import org.apache.htrace.HTraceConfiguration; + import org.apache.htrace.Sampler; +-import org.apache.htrace.Span; + import org.apache.htrace.SpanReceiver; + import org.apache.htrace.SpanReceiverBuilder; + import org.apache.htrace.Trace; + import org.apache.htrace.TraceScope; +-import org.junit.Ignore; + import org.junit.Test; + import static org.junit.Assert.assertTrue; + import static org.junit.Assert.assertFalse; +diff --git a/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java b/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java +new file mode 100644 +index 0000000..8f26bf9 +--- /dev/null ++++ b/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java +@@ -0,0 +1,101 @@ ++package org.apache.htrace.wrappers; ++ ++import static org.junit.Assert.assertSame; ++import static org.junit.Assert.assertTrue; ++import static org.mockito.Mockito.mock; ++ ++import java.util.concurrent.Callable; ++import java.util.concurrent.ExecutorService; ++ ++import org.junit.Before; ++import org.junit.Test; ++import org.mockito.ArgumentCaptor; ++import org.mockito.Captor; ++import org.mockito.Mockito; ++import org.mockito.MockitoAnnotations; ++ ++public class TestTraceExecutorService { ++ ++ @Captor ++ private ArgumentCaptor runnableCaptor; ++ @Captor ++ private ArgumentCaptor> callableCaptor; ++ ++ @Before ++ public void init(){ ++ MockitoAnnotations.initMocks(this); ++ } ++ ++ @Test ++ public void wrapRunnable() { ++ ++ Runnable mockedRunable = mock(Runnable.class); ++ ++ Runnable wrappedRunnable = TraceExecutorService.wrap(mockedRunable); ++ assertTrue(wrappedRunnable instanceof TraceRunnable); ++ TraceRunnable wrappedTraceRunnable = (TraceRunnable) wrappedRunnable; ++ assertSame(mockedRunable, wrappedTraceRunnable.getRunnable()); ++ ++ TraceRunnable traceRunnable = new TraceRunnable(mockedRunable); ++ wrappedRunnable = TraceExecutorService.wrap(traceRunnable); ++ assertTrue(wrappedRunnable instanceof TraceRunnable); ++ wrappedTraceRunnable = (TraceRunnable) wrappedRunnable; ++ assertSame(traceRunnable, wrappedTraceRunnable); ++ assertSame(mockedRunable, wrappedTraceRunnable.getRunnable()); ++ } ++ ++ @Test ++ public void wrapCallable() { ++ ++ @SuppressWarnings("unchecked") ++ Callable mockedCallable = (Callable)Mockito.mock(Callable.class); ++ ++ Callable wrappedCallable = TraceExecutorService.wrap(mockedCallable); ++ assertTrue(wrappedCallable instanceof TraceCallable); ++ TraceCallable wrappedTraceCallable = (TraceCallable) wrappedCallable; ++ assertSame(mockedCallable, wrappedTraceCallable.getImpl()); ++ ++ TraceCallable traceRunnable = new TraceCallable(mockedCallable); ++ wrappedCallable = TraceExecutorService.wrap(traceRunnable); ++ assertTrue(wrappedCallable instanceof TraceCallable); ++ wrappedTraceCallable = (TraceCallable) wrappedCallable; ++ assertSame(traceRunnable, wrappedTraceCallable); ++ assertSame(mockedCallable, wrappedTraceCallable.getImpl()); ++ } ++ ++ @Test ++ public void submitRunnable() { ++ Runnable mockedRunable = Mockito.mock(Runnable.class); ++ ++ ExecutorService mockExecutor = mock(ExecutorService.class); ++ ++ TraceExecutorService traceExecutorService = new TraceExecutorService(mockExecutor); ++ ++ traceExecutorService.submit(mockedRunable); ++ ++ Mockito.verify(mockExecutor).submit(runnableCaptor.capture()); ++ ++ Runnable submittedRunnable = runnableCaptor.getValue(); ++ assertTrue(submittedRunnable instanceof TraceRunnable); ++ TraceRunnable traceRunnable = (TraceRunnable) submittedRunnable; ++ assertSame(mockedRunable, traceRunnable.getRunnable()); ++ } ++ ++ @Test ++ public void submitCallable() { ++ @SuppressWarnings("unchecked") ++ Callable mockedCallable = (Callable)Mockito.mock(Callable.class); ++ ++ ExecutorService mockExecutor = mock(ExecutorService.class); ++ TraceExecutorService traceExecutorService = new TraceExecutorService(mockExecutor); ++ ++ traceExecutorService.submit(mockedCallable); ++ ++ Mockito.verify(mockExecutor).submit(callableCaptor.capture()); ++ ++ Callable submittedCallable = callableCaptor.getValue(); ++ assertTrue(submittedCallable instanceof TraceCallable); ++ TraceCallable traceCallable = (TraceCallable) submittedCallable; ++ assertSame(mockedCallable, traceCallable.getImpl()); ++ } ++} +diff --git a/pom.xml b/pom.xml +index 83c38d2..8b41d8e 100644 +--- a/pom.xml ++++ b/pom.xml +@@ -301,6 +301,12 @@ language governing permissions and limitations under the License. --> + jackson-databind + 2.4.0 + ++ ++ org.mockito ++ mockito-core ++ 1.9.5 ++ test ++ + + + +-- +2.3.5 + diff --git a/htrace-core/pom.xml b/htrace-core/pom.xml index 6026510..5c4715d 100644 --- a/htrace-core/pom.xml +++ b/htrace-core/pom.xml @@ -104,6 +104,11 @@ language governing permissions and limitations under the License. --> commons-logging commons-logging + + org.mockito + mockito-core + test + diff --git a/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java b/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java index 03e891f..5776354 100644 --- a/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java +++ b/htrace-core/src/main/java/org/apache/htrace/wrappers/TraceExecutorService.java @@ -37,7 +37,7 @@ public class TraceExecutorService implements ExecutorService { @Override public void execute(Runnable command) { - impl.execute(new TraceRunnable(command)); + impl.execute(wrap(command)); } @Override @@ -68,24 +68,40 @@ public class TraceExecutorService implements ExecutorService { @Override public Future submit(Callable task) { - return impl.submit(new TraceCallable(task)); + return impl.submit(wrap(task)); } @Override public Future submit(Runnable task, T result) { - return impl.submit(new TraceRunnable(task), result); + return impl.submit(wrap(task), result); } @Override public Future submit(Runnable task) { - return impl.submit(new TraceRunnable(task)); + return impl.submit(wrap(task)); + } + + static Runnable wrap(Runnable task) { + if (task instanceof TraceRunnable) { + return task; + } + + return new TraceRunnable(task); + } + + static Callable wrap(Callable task) { + if (task instanceof TraceCallable) { + return task; + } + + return new TraceCallable(task); } private Collection> wrapCollection( Collection> tasks) { List> result = new ArrayList>(); for (Callable task : tasks) { - result.add(new TraceCallable(task)); + result.add(wrap(task)); } return result; } diff --git a/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java b/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java index 634bef8..219e97a 100644 --- a/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java +++ b/htrace-core/src/test/java/org/apache/htrace/impl/TestLocalFileSpanReceiver.java @@ -22,12 +22,10 @@ import java.io.IOException; import java.util.HashMap; import org.apache.htrace.HTraceConfiguration; import org.apache.htrace.Sampler; -import org.apache.htrace.Span; import org.apache.htrace.SpanReceiver; import org.apache.htrace.SpanReceiverBuilder; import org.apache.htrace.Trace; import org.apache.htrace.TraceScope; -import org.junit.Ignore; import org.junit.Test; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertFalse; diff --git a/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java b/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java new file mode 100644 index 0000000..5db0102 --- /dev/null +++ b/htrace-core/src/test/java/org/apache/htrace/wrappers/TestTraceExecutorService.java @@ -0,0 +1,99 @@ +package org.apache.htrace.wrappers; + +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; + +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +public class TestTraceExecutorService { + + @Captor + private ArgumentCaptor runnableCaptor; + @Captor + private ArgumentCaptor> callableCaptor; + + @Before + public void before(){ + MockitoAnnotations.initMocks(this); + } + + @Test + public void wrapRunnable() { + Runnable mockedRunable = mock(Runnable.class); + + Runnable wrappedRunnable = TraceExecutorService.wrap(mockedRunable); + assertTrue(wrappedRunnable instanceof TraceRunnable); + TraceRunnable wrappedTraceRunnable = (TraceRunnable) wrappedRunnable; + assertSame(mockedRunable, wrappedTraceRunnable.getRunnable()); + + TraceRunnable traceRunnable = new TraceRunnable(mockedRunable); + wrappedRunnable = TraceExecutorService.wrap(traceRunnable); + assertTrue(wrappedRunnable instanceof TraceRunnable); + wrappedTraceRunnable = (TraceRunnable) wrappedRunnable; + assertSame(traceRunnable, wrappedTraceRunnable); + assertSame(mockedRunable, wrappedTraceRunnable.getRunnable()); + } + + @Test + public void wrapCallable() { + @SuppressWarnings("unchecked") + Callable mockedCallable = (Callable)Mockito.mock(Callable.class); + + Callable wrappedCallable = TraceExecutorService.wrap(mockedCallable); + assertTrue(wrappedCallable instanceof TraceCallable); + TraceCallable wrappedTraceCallable = (TraceCallable) wrappedCallable; + assertSame(mockedCallable, wrappedTraceCallable.getImpl()); + + TraceCallable traceRunnable = new TraceCallable(mockedCallable); + wrappedCallable = TraceExecutorService.wrap(traceRunnable); + assertTrue(wrappedCallable instanceof TraceCallable); + wrappedTraceCallable = (TraceCallable) wrappedCallable; + assertSame(traceRunnable, wrappedTraceCallable); + assertSame(mockedCallable, wrappedTraceCallable.getImpl()); + } + + @Test + public void submitRunnable() { + Runnable mockedRunable = Mockito.mock(Runnable.class); + + ExecutorService mockExecutor = mock(ExecutorService.class); + + TraceExecutorService traceExecutorService = new TraceExecutorService(mockExecutor); + + traceExecutorService.submit(mockedRunable); + + Mockito.verify(mockExecutor).submit(runnableCaptor.capture()); + + Runnable submittedRunnable = runnableCaptor.getValue(); + assertTrue(submittedRunnable instanceof TraceRunnable); + TraceRunnable traceRunnable = (TraceRunnable) submittedRunnable; + assertSame(mockedRunable, traceRunnable.getRunnable()); + } + + @Test + public void submitCallable() { + @SuppressWarnings("unchecked") + Callable mockedCallable = (Callable)Mockito.mock(Callable.class); + + ExecutorService mockExecutor = mock(ExecutorService.class); + TraceExecutorService traceExecutorService = new TraceExecutorService(mockExecutor); + + traceExecutorService.submit(mockedCallable); + + Mockito.verify(mockExecutor).submit(callableCaptor.capture()); + + Callable submittedCallable = callableCaptor.getValue(); + assertTrue(submittedCallable instanceof TraceCallable); + TraceCallable traceCallable = (TraceCallable) submittedCallable; + assertSame(mockedCallable, traceCallable.getImpl()); + } +} diff --git a/pom.xml b/pom.xml index 83c38d2..8b41d8e 100644 --- a/pom.xml +++ b/pom.xml @@ -301,6 +301,12 @@ language governing permissions and limitations under the License. --> jackson-databind 2.4.0 + + org.mockito + mockito-core + 1.9.5 + test + -- 2.3.5