From 372d8faa46d7f3a0a894776f3d499d8ea6a188ac Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 13 Dec 2017 10:21:29 +0800 Subject: [PATCH] Corrects missing status code in servlet 2.5 environments Before, we couldn't detect the status code unless we wrapped it first. This made layered tools on servlet api not work unless they could affect the servlet response. For example, Spring MVC's interceptor cannot. Benchmarking was used to show that caching the reflective lookup saves an order of magnitude of overhead than doing so otherwise. --- .../servlet/ServletRuntimeBenchmarks.java | 289 ++++++++++++++++++ .../brave/servlet/ServletRuntime25Test.java | 154 ++++++++++ .../java/brave/servlet/ServletRuntime.java | 56 +++- .../brave/servlet/ServletRuntimeTest.java | 231 ++++++++++++++ 4 files changed, 727 insertions(+), 3 deletions(-) create mode 100644 instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java create mode 100644 instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java create mode 100644 instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java diff --git a/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java new file mode 100644 index 0000000000..556463f7d5 --- /dev/null +++ b/instrumentation/benchmarks/src/main/java/brave/servlet/ServletRuntimeBenchmarks.java @@ -0,0 +1,289 @@ +/** + * Copyright 2015-2016 The OpenZipkin Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package brave.servlet; + +import java.io.PrintWriter; +import java.util.Collection; +import java.util.Locale; +import java.util.concurrent.TimeUnit; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Group; +import org.openjdk.jmh.annotations.GroupThreads; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +@Measurement(iterations = 5, time = 1) +@Warmup(iterations = 10, time = 1) +@Fork(3) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +public class ServletRuntimeBenchmarks { + static final ServletRuntime servlet3 = new ServletRuntime.Servlet3(); + static final ServletRuntime servlet25 = new ServletRuntime.Servlet25(); + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_servlet3() { + return threeStatuses(servlet3); + } + + int threeStatuses(ServletRuntime runtime) { + // & to ensure null wasn't returned (forces NPE) + return runtime.status(new Response1()) & + runtime.status(new Response2()) & + runtime.status(new Response3()); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_httpResponse_servlet3() { + return threeResponses(servlet3); + } + + int threeResponses(ServletRuntime runtime) { + // & to ensure null wasn't returned (forces NPE) + return runtime.httpResponse(new Response1()).getStatus() & + runtime.httpResponse(new Response2()).getStatus() & + runtime.httpResponse(new Response3()).getStatus(); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_servlet25() { + return threeStatuses(servlet25); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_httpResponse_servlet25() { + return threeResponses(servlet25); + } + + @Benchmark @Group("no_contention") @GroupThreads(1) + public int no_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + @Benchmark @Group("mild_contention") @GroupThreads(2) + public int mild_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + @Benchmark @Group("high_contention") @GroupThreads(8) + public int high_contention_status_reflection() throws Exception { + return threeStatusesReflection(); + } + + private int threeStatusesReflection() throws Exception { + // & to ensure null wasn't returned (forces NPE) + return ((int) Response1.class.getMethod("getStatus").invoke(new Response1())) & + ((int) Response2.class.getMethod("getStatus").invoke(new Response2())) & + ((int) Response3.class.getMethod("getStatus").invoke(new Response3())); + } + + // Convenience main entry-point + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include(".*" + ServletRuntimeBenchmarks.class.getSimpleName() + ".*") + .build(); + + new Runner(opt).run(); + } + + class Response1 extends HttpServletResponseImpl { + } + + class Response2 extends HttpServletResponseImpl { + } + + class Response3 extends HttpServletResponseImpl { + } + + public static class HttpServletResponseImpl implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() { + return null; + } + + @Override public PrintWriter getWriter() { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentLengthLong(long len) { + + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) { + } + + @Override public void sendError(int sc) { + } + + @Override public void sendRedirect(String location) { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + + @Override public int getStatus() { + return 200; + } + + @Override public String getHeader(String name) { + return null; + } + + @Override public Collection getHeaders(String name) { + return null; + } + + @Override public Collection getHeaderNames() { + return null; + } + } +} diff --git a/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java b/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java new file mode 100644 index 0000000000..48b2eb54dc --- /dev/null +++ b/instrumentation/servlet/src/it/servlet25/src/test/java/brave/servlet/ServletRuntime25Test.java @@ -0,0 +1,154 @@ +package brave.servlet; + +import java.io.IOException; +import java.io.PrintWriter; +import java.lang.reflect.Field; +import java.util.Locale; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.Response; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** These tests must to be run in a classpath without servlet 3.x types */ +public class ServletRuntime25Test { + + @Test public void status_fromJetty() throws Exception { + Response jettyResponse = new Response(null); + Field field = Response.class.getDeclaredField("_status"); + field.setAccessible(true); + field.set(jettyResponse, 400); + assertThat(ServletRuntime.get().status(jettyResponse)) + .isEqualTo(400); + } + + @Test public void httpResponse_wrapsHttpServletResponse() throws Exception { + assertThat(ServletRuntime.get().httpResponse(new WithoutGetStatus())) + .isInstanceOf(ServletRuntime.Servlet25ServerResponseAdapter.class); + } + + @Test public void status_fromInvalidMethod() throws Exception { + assertThat(ServletRuntime.get().status(new WithInvalidGetStatus())) + .isNull(); + } + + public static class WithInvalidGetStatus extends WithoutGetStatus { + public String getStatus(){ + return "foo"; + } + } + + // intentionally won't compile if servlet > 2.5 is present! testing handling of missing getStatus + public static class WithoutGetStatus implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() throws IOException { + return null; + } + + @Override public PrintWriter getWriter() throws IOException { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() throws IOException { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) throws IOException { + } + + @Override public void sendError(int sc) throws IOException { + } + + @Override public void sendRedirect(String location) throws IOException { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + } +} diff --git a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java index 4293a617da..1be349ac80 100644 --- a/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java +++ b/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java @@ -4,6 +4,10 @@ import brave.http.HttpServerHandler; import brave.internal.Nullable; import java.io.IOException; +import java.lang.reflect.Method; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import javax.servlet.AsyncContext; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; @@ -11,6 +15,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; +import zipkin2.Call; /** * Access to servlet version-specific features @@ -55,7 +60,7 @@ private static ServletRuntime findServletRuntime() { return new Servlet25(); } - private static final class Servlet3 extends ServletRuntime { + static final class Servlet3 extends ServletRuntime { @Override boolean isAsync(HttpServletRequest request) { return request.isAsyncStarted(); } @@ -112,7 +117,7 @@ static final class TracingAsyncListener implements AsyncListener { } } - private static final class Servlet25 extends ServletRuntime { + static final class Servlet25 extends ServletRuntime { @Override HttpServletResponse httpResponse(ServletResponse response) { return new Servlet25ServerResponseAdapter(response); } @@ -126,12 +131,57 @@ private static final class Servlet25 extends ServletRuntime { assert false : "this should never be called in Servlet 2.5"; } + // copy-on-write global reflection cache outperforms thread local copies + final AtomicReference, Object>> classToGetStatus = + new AtomicReference<>(new LinkedHashMap<>()); + static final String RETURN_NULL = "RETURN_NULL"; + + /** + * Eventhough the Servlet 2.5 version of HttpServletResponse doesn't have the getStatus method, + * routine servlet runtimes, do, for example {@code org.eclipse.jetty.server.Response} + */ @Override @Nullable Integer status(HttpServletResponse response) { if (response instanceof Servlet25ServerResponseAdapter) { // servlet 2.5 doesn't have get status return ((Servlet25ServerResponseAdapter) response).getStatusInServlet25(); } - return null; + Class clazz = response.getClass(); + Map, Object> classesToCheck = classToGetStatus.get(); + Object getStatusMethod = classesToCheck.get(clazz); + if (getStatusMethod == RETURN_NULL || + (getStatusMethod == null && classesToCheck.size() == 10)) { // limit size + return null; + } + + // Now, we either have a cached method or we have room to cache a method + if (getStatusMethod == null) { + if (clazz.isLocalClass() || clazz.isAnonymousClass()) return null; // don't cache + try { + // we don't check for accessibility as isAccessible is deprecated: just fail later + getStatusMethod = clazz.getMethod("getStatus"); + return (int) ((Method) getStatusMethod).invoke(response); + } catch (Throwable throwable) { + Call.propagateIfFatal(throwable); + getStatusMethod = RETURN_NULL; + return null; + } finally { + // regardless of success or fail, replace the cache + Map, Object> replacement = new LinkedHashMap<>(classesToCheck); + replacement.put(clazz, getStatusMethod); + classToGetStatus.set(replacement); // lost race will reset, but only up to size - 1 times + } + } + + // if we are here, we have a cached method, that "should" never fail, but we check anyway + try { + return (int) ((Method) getStatusMethod).invoke(response); + } catch (Throwable throwable) { + Call.propagateIfFatal(throwable); + Map, Object> replacement = new LinkedHashMap<>(classesToCheck); + replacement.put(clazz, RETURN_NULL); + classToGetStatus.set(replacement); // prefer overriding on failure + return null; + } } } diff --git a/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java b/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java new file mode 100644 index 0000000000..e236bd2e3f --- /dev/null +++ b/instrumentation/servlet/src/test/java/brave/servlet/ServletRuntimeTest.java @@ -0,0 +1,231 @@ +package brave.servlet; + +import java.io.PrintWriter; +import java.util.Collection; +import java.util.Locale; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.Response; +import org.junit.Before; +import org.junit.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ServletRuntimeTest { + ServletRuntime servlet25 = new ServletRuntime.Servlet25(); + + @Test public void servlet25_status_doesntParseAnonymousTypes() throws Exception { + // while looks nice, this will overflow our cache + Response jettyResponse = new Response(null) { + @Override public int getStatus() { + throw new AssertionError(); + } + }; + assertThat(servlet25.status(jettyResponse)) + .isNull(); + } + + @Test public void servlet25_status_doesntParseLocalTypes() throws Exception { + // while looks nice, this will overflow our cache + class LocalResponse extends HttpServletResponseImpl { + } + assertThat(servlet25.status(new LocalResponse())) + .isNull(); + } + + class ExceptionResponse extends HttpServletResponseImpl { + @Override public int getStatus() { + throw new IllegalStateException("foo"); + } + } + + @Test public void servlet25_status_nullOnException() throws Exception { + assertThat(servlet25.status(new ExceptionResponse())) + .isNull(); + } + + class Response1 extends HttpServletResponseImpl { + } + + class Response2 extends HttpServletResponseImpl { + } + + class Response3 extends HttpServletResponseImpl { + } + + class Response4 extends HttpServletResponseImpl { + } + + class Response5 extends HttpServletResponseImpl { + } + + class Response6 extends HttpServletResponseImpl { + } + + class Response7 extends HttpServletResponseImpl { + } + + class Response8 extends HttpServletResponseImpl { + } + + class Response9 extends HttpServletResponseImpl { + } + + class Response10 extends HttpServletResponseImpl { + } + + class Response11 extends HttpServletResponseImpl { + } + + @Test public void servlet25_status_cachesUpToTenTypes() throws Exception { + assertThat(servlet25.status(new Response1())) + .isEqualTo(200); + assertThat(servlet25.status(new Response2())) + .isEqualTo(200); + assertThat(servlet25.status(new Response3())) + .isEqualTo(200); + assertThat(servlet25.status(new Response4())) + .isEqualTo(200); + assertThat(servlet25.status(new Response5())) + .isEqualTo(200); + assertThat(servlet25.status(new Response6())) + .isEqualTo(200); + assertThat(servlet25.status(new Response7())) + .isEqualTo(200); + assertThat(servlet25.status(new Response8())) + .isEqualTo(200); + assertThat(servlet25.status(new Response9())) + .isEqualTo(200); + assertThat(servlet25.status(new Response10())) + .isEqualTo(200); + assertThat(servlet25.status(new Response11())) + .isNull(); + } + + public static class HttpServletResponseImpl implements HttpServletResponse { + @Override public String getCharacterEncoding() { + return null; + } + + @Override public String getContentType() { + return null; + } + + @Override public ServletOutputStream getOutputStream() { + return null; + } + + @Override public PrintWriter getWriter() { + return null; + } + + @Override public void setCharacterEncoding(String charset) { + } + + @Override public void setContentLength(int len) { + } + + @Override public void setContentType(String type) { + } + + @Override public void setBufferSize(int size) { + } + + @Override public int getBufferSize() { + return 0; + } + + @Override public void flushBuffer() { + } + + @Override public void resetBuffer() { + } + + @Override public boolean isCommitted() { + return false; + } + + @Override public void reset() { + } + + @Override public void setLocale(Locale loc) { + } + + @Override public Locale getLocale() { + return null; + } + + @Override public void addCookie(Cookie cookie) { + } + + @Override public boolean containsHeader(String name) { + return false; + } + + @Override public String encodeURL(String url) { + return null; + } + + @Override public String encodeRedirectURL(String url) { + return null; + } + + @Override public String encodeUrl(String url) { + return null; + } + + @Override public String encodeRedirectUrl(String url) { + return null; + } + + @Override public void sendError(int sc, String msg) { + } + + @Override public void sendError(int sc) { + } + + @Override public void sendRedirect(String location) { + } + + @Override public void setDateHeader(String name, long date) { + } + + @Override public void addDateHeader(String name, long date) { + } + + @Override public void setHeader(String name, String value) { + } + + @Override public void addHeader(String name, String value) { + } + + @Override public void setIntHeader(String name, int value) { + } + + @Override public void addIntHeader(String name, int value) { + } + + @Override public void setStatus(int sc) { + } + + @Override public void setStatus(int sc, String sm) { + } + + @Override public int getStatus() { + return 200; + } + + @Override public String getHeader(String name) { + return null; + } + + @Override public Collection getHeaders(String name) { + return null; + } + + @Override public Collection getHeaderNames() { + return null; + } + } +}