honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Jay Modi 2017-03-02 14:44:01 -05:00 committed by GitHub
parent 577d2a6a1d
commit 01502893eb
5 changed files with 62 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
core/src/main/java/org/elasticsearch/rest/RestController.java
View File

@ -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);
}
}
}

11
core/src/test/java/org/elasticsearch/rest/RestControllerTests.java
View File

@ -97,20 +97,21 @@ public class RestControllerTests extends ESTestCase {
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
Set<String> 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<String, List<String>> 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 {

1
modules/transport-netty4/build.gradle
View File

@ -29,6 +29,7 @@ esplugin {
hasClientJar = true
}
compileJava.options.compilerArgs << "-Xlint:-try"
compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked"
dependencies {

10
modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java
View File

@ -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 {

33
modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java
View File

@ -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"));
}
}
}