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

Stop Recreating Wrapped Handlers in RestController (#44964) (#45040) 

* We shouldn't be recreating wrapped REST handlers over and over for every request. We only use this hook in x-pack and the wrapper there does not have any per request state.
  This is inefficient and could lead to some very unexpected memory behavior
   => I made the logic create the wrapper on handler registration and adjusted the x-pack wrapper implementation to correctly forward the circuit breaker and content stream flags
This commit is contained in:
Armin Braun 2019-07-31 17:11:34 +02:00 committed by GitHub
parent 56da35b706
commit c7d7230524
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 29 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
server/src/main/java/org/elasticsearch/rest/RestController.java
View File

@ -152,8 +152,9 @@ public class RestController implements HttpServerTransport.Dispatcher {
if (handler instanceof BaseRestHandler) {
usageService.addRestHandler((BaseRestHandler) handler);
}
handlers.insertOrUpdate(path, new MethodHandlers(path, handler, method), (mHandlers, newMHandler) -> {
return mHandlers.addMethods(handler, method);
final RestHandler maybeWrappedHandler = handlerWrapper.apply(handler);
handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), (mHandlers, newMHandler) -> {
return mHandlers.addMethods(maybeWrappedHandler, method);
});
}
@ -236,7 +237,7 @@ public class RestController implements HttpServerTransport.Dispatcher {
// iff we could reserve bytes for the request we need to send the response also over this channel
responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength);
final RestHandler wrappedHandler = mHandler.map(h -> handlerWrapper.apply(h)).get();
final RestHandler wrappedHandler = mHandler.get();
wrappedHandler.handleRequest(request, responseChannel, client);
requestHandled = true;
} catch (Exception e) {

25
server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
View File

@ -57,7 +57,6 @@ import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.UnaryOperator;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
@ -80,7 +79,6 @@ public class RestControllerTests extends ESTestCase {
@Before
public void setup() {
Settings settings = Settings.EMPTY;
circuitBreakerService = new HierarchyCircuitBreakerService(
Settings.builder()
.put(HierarchyCircuitBreakerService.IN_FLIGHT_REQUESTS_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(), BREAKER_LIMIT)
@ -205,16 +203,19 @@ public class RestControllerTests extends ESTestCase {
public void testRestHandlerWrapper() throws Exception {
AtomicBoolean handlerCalled = new AtomicBoolean(false);
AtomicBoolean wrapperCalled = new AtomicBoolean(false);
RestHandler handler = (RestRequest request, RestChannel channel, NodeClient client) -> {
handlerCalled.set(true);
};
UnaryOperator<RestHandler> wrapper = h -> {
assertSame(handler, h);
return (RestRequest request, RestChannel channel, NodeClient client) -> wrapperCalled.set(true);
};
final RestController restController = new RestController(Collections.emptySet(), wrapper, null,
circuitBreakerService, usageService);
restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(), null, null, Optional.of(handler));
final RestHandler handler = (RestRequest request, RestChannel channel, NodeClient client) -> handlerCalled.set(true);
final HttpServerTransport httpServerTransport = new TestHttpServerTransport();
final RestController restController =
new RestController(Collections.emptySet(),
h -> {
assertSame(handler, h);
return (RestRequest request, RestChannel channel, NodeClient client) -> wrapperCalled.set(true);
}, null, circuitBreakerService, usageService);
restController.registerHandler(RestRequest.Method.GET, "/wrapped", handler);
RestRequest request = testRestRequest("/wrapped", "{}", XContentType.JSON);
AssertingChannel channel = new AssertingChannel(request, true, RestStatus.BAD_REQUEST);
restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY));
httpServerTransport.start();
assertTrue(wrapperCalled.get());
assertFalse(handlerCalled.get());
}

14
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java
View File

@ -5,8 +5,8 @@
*/
package org.elasticsearch.xpack.security.rest;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.logging.log4j.util.Supplier;
import org.elasticsearch.action.ActionListener;
@ -70,7 +70,17 @@ public class SecurityRestFilter implements RestHandler {
}
}
RestRequest maybeWrapRestRequest(RestRequest restRequest) throws IOException {
@Override
public boolean canTripCircuitBreaker() {
return restHandler.canTripCircuitBreaker();
}
@Override
public boolean supportsContentStream() {
return restHandler.supportsContentStream();
}
private RestRequest maybeWrapRestRequest(RestRequest restRequest) throws IOException {
if (restHandler instanceof RestRequestFilter) {
return ((RestRequestFilter)restHandler).getFilteredRequest(restRequest);
}