From 01502893eb13acd0c128756c1066b2314bbb1b0d Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Thu, 2 Mar 2017 14:44:01 -0500 Subject: [PATCH] HTTP transport stashes the ThreadContext instead of the RestController (#23456) Previously, the RestController would stash the context prior to copying headers. However, there could be deprecation log messages logged and in turn warning headers being added to the context prior to the stashing of the context. These headers in the context would then be removed from the request and also leaked back into the calling thread's context. This change moves the stashing of the context to the HttpTransport so that the network threads' context isn't accidentally populated with warning headers and to ensure the headers added early on in the RestController are not excluded from the response. --- .../elasticsearch/rest/RestController.java | 30 ++++++++--------- .../rest/RestControllerTests.java | 11 ++++--- modules/transport-netty4/build.gradle | 1 + .../netty4/Netty4HttpServerTransport.java | 10 ++++-- .../Netty4HttpServerTransportTests.java | 33 +++++++++++++++++++ 5 files changed, 62 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/rest/RestController.java b/core/src/main/java/org/elasticsearch/rest/RestController.java index 8e67a2e7441..ea603cf949f 100644 --- a/core/src/main/java/org/elasticsearch/rest/RestController.java +++ b/core/src/main/java/org/elasticsearch/rest/RestController.java @@ -231,27 +231,25 @@ public class RestController extends AbstractComponent implements HttpServerTrans if (checkRequestParameters(request, channel) == false) { channel.sendResponse(BytesRestResponse.createSimpleErrorResponse(BAD_REQUEST, "error traces in responses are disabled.")); } else { - try (ThreadContext.StoredContext ignored = threadContext.stashContext()) { - for (String key : headersToCopy) { - String httpHeader = request.header(key); - if (httpHeader != null) { - threadContext.putHeader(key, httpHeader); - } + for (String key : headersToCopy) { + String httpHeader = request.header(key); + if (httpHeader != null) { + threadContext.putHeader(key, httpHeader); } + } - if (handler == null) { - if (request.method() == RestRequest.Method.OPTIONS) { - // when we have OPTIONS request, simply send OK by default (with the Access Control Origin header which gets automatically added) + if (handler == null) { + if (request.method() == RestRequest.Method.OPTIONS) { + // when we have OPTIONS request, simply send OK by default (with the Access Control Origin header which gets automatically added) - channel.sendResponse(new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); - } else { - final String msg = "No handler found for uri [" + request.uri() + "] and method [" + request.method() + "]"; - channel.sendResponse(new BytesRestResponse(BAD_REQUEST, msg)); - } + channel.sendResponse(new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); } else { - final RestHandler wrappedHandler = Objects.requireNonNull(handlerWrapper.apply(handler)); - wrappedHandler.handleRequest(request, channel, client); + final String msg = "No handler found for uri [" + request.uri() + "] and method [" + request.method() + "]"; + channel.sendResponse(new BytesRestResponse(BAD_REQUEST, msg)); } + } else { + final RestHandler wrappedHandler = Objects.requireNonNull(handlerWrapper.apply(handler)); + wrappedHandler.handleRequest(request, channel, client); } } } diff --git a/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java index cc035ecc103..1e279a75218 100644 --- a/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -97,20 +97,21 @@ public class RestControllerTests extends ESTestCase { final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); Set headers = new HashSet<>(Arrays.asList("header.1", "header.2")); final RestController restController = new RestController(Settings.EMPTY, headers, null, null, circuitBreakerService); - threadContext.putHeader("header.3", "true"); Map> restHeaders = new HashMap<>(); restHeaders.put("header.1", Collections.singletonList("true")); restHeaders.put("header.2", Collections.singletonList("true")); restHeaders.put("header.3", Collections.singletonList("false")); restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).withHeaders(restHeaders).build(), null, null, - threadContext, (RestRequest request, RestChannel channel, NodeClient client) -> { + threadContext, (RestRequest request, RestChannel channel, NodeClient client) -> { assertEquals("true", threadContext.getHeader("header.1")); assertEquals("true", threadContext.getHeader("header.2")); assertNull(threadContext.getHeader("header.3")); }); - assertNull(threadContext.getHeader("header.1")); - assertNull(threadContext.getHeader("header.2")); - assertEquals("true", threadContext.getHeader("header.3")); + // the rest controller relies on the caller to stash the context, so we should expect these values here as we didn't stash the + // context in this test + assertEquals("true", threadContext.getHeader("header.1")); + assertEquals("true", threadContext.getHeader("header.2")); + assertNull(threadContext.getHeader("header.3")); } public void testCanTripCircuitBreaker() throws Exception { diff --git a/modules/transport-netty4/build.gradle b/modules/transport-netty4/build.gradle index 7c70b114647..6afea402c6e 100644 --- a/modules/transport-netty4/build.gradle +++ b/modules/transport-netty4/build.gradle @@ -29,6 +29,7 @@ esplugin { hasClientJar = true } +compileJava.options.compilerArgs << "-Xlint:-try" compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked" dependencies { diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java index be1c840c516..0f7c45f4eb6 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java @@ -486,11 +486,17 @@ public class Netty4HttpServerTransport extends AbstractLifecycleComponent implem } void dispatchRequest(final RestRequest request, final RestChannel channel) { - dispatcher.dispatchRequest(request, channel, threadPool.getThreadContext()); + final ThreadContext threadContext = threadPool.getThreadContext(); + try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + dispatcher.dispatchRequest(request, channel, threadContext); + } } void dispatchBadRequest(final RestRequest request, final RestChannel channel, final Throwable cause) { - dispatcher.dispatchBadRequest(request, channel, threadPool.getThreadContext(), cause); + final ThreadContext threadContext = threadPool.getThreadContext(); + try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + dispatcher.dispatchBadRequest(request, channel, threadContext, cause); + } } protected void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java index 9af77e576ff..7a942f8853a 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java @@ -282,4 +282,37 @@ public class Netty4HttpServerTransportTests extends ESTestCase { assertThat(causeReference.get(), instanceOf(TooLongFrameException.class)); } + public void testDispatchDoesNotModifyThreadContext() throws InterruptedException { + final HttpServerTransport.Dispatcher dispatcher = new HttpServerTransport.Dispatcher() { + + @Override + public void dispatchRequest(final RestRequest request, final RestChannel channel, final ThreadContext threadContext) { + threadContext.putHeader("foo", "bar"); + threadContext.putTransient("bar", "baz"); + } + + @Override + public void dispatchBadRequest(final RestRequest request, + final RestChannel channel, + final ThreadContext threadContext, + final Throwable cause) { + threadContext.putHeader("foo_bad", "bar"); + threadContext.putTransient("bar_bad", "baz"); + } + + }; + + try (Netty4HttpServerTransport transport = + new Netty4HttpServerTransport(Settings.EMPTY, networkService, bigArrays, threadPool, xContentRegistry(), dispatcher)) { + transport.start(); + + transport.dispatchRequest(null, null); + assertNull(threadPool.getThreadContext().getHeader("foo")); + assertNull(threadPool.getThreadContext().getTransient("bar")); + + transport.dispatchBadRequest(null, null, null); + assertNull(threadPool.getThreadContext().getHeader("foo_bad")); + assertNull(threadPool.getThreadContext().getTransient("bar_bad")); + } + } }