From 00e4fba2fb0ac124e5d166daba42be6da6022de4 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Tue, 13 Aug 2019 10:43:30 +0200 Subject: [PATCH] Simplify and Optimize RestController Slightly (#45419) (#45485) * Simplify the path iterator to generate less garbage * `dispatchRequest` always terminates, adjust code accordingly --- .../elasticsearch/common/path/PathTrie.java | 49 ++++++------------- .../elasticsearch/rest/RestController.java | 34 ++++++------- 2 files changed, 30 insertions(+), 53 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/path/PathTrie.java b/server/src/main/java/org/elasticsearch/common/path/PathTrie.java index 8f6abc3102f..7b6c660a64a 100644 --- a/server/src/main/java/org/elasticsearch/common/path/PathTrie.java +++ b/server/src/main/java/org/elasticsearch/common/path/PathTrie.java @@ -19,12 +19,9 @@ package org.elasticsearch.common.path; -import java.util.ArrayList; -import java.util.Arrays; import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; -import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.function.BiFunction; @@ -352,40 +349,22 @@ public class PathTrie { * of parameters. */ public Iterator retrieveAll(String path, Supplier> paramSupplier) { - return new PathTrieIterator<>(this, path, paramSupplier); - } + return new Iterator() { - private static class PathTrieIterator implements Iterator { + private int mode; - private final List modes; - private final Supplier> paramSupplier; - private final PathTrie trie; - private final String path; - - PathTrieIterator(PathTrie trie, String path, Supplier> paramSupplier) { - this.path = path; - this.trie = trie; - this.paramSupplier = paramSupplier; - this.modes = new ArrayList<>(Arrays.asList(TrieMatchingMode.EXPLICIT_NODES_ONLY, - TrieMatchingMode.WILDCARD_ROOT_NODES_ALLOWED, - TrieMatchingMode.WILDCARD_LEAF_NODES_ALLOWED, - TrieMatchingMode.WILDCARD_NODES_ALLOWED)); - assert TrieMatchingMode.values().length == 4 : "missing trie matching mode"; - } - - @Override - public boolean hasNext() { - return modes.isEmpty() == false; - } - - @Override - public T next() { - if (modes.isEmpty()) { - throw new NoSuchElementException("called next() without validating hasNext()! no more modes available"); + @Override + public boolean hasNext() { + return mode < TrieMatchingMode.values().length; } - TrieMatchingMode mode = modes.remove(0); - Map params = paramSupplier.get(); - return trie.retrieve(path, params, mode); - } + + @Override + public T next() { + if (hasNext() == false) { + throw new NoSuchElementException("called next() without validating hasNext()! no more modes available"); + } + return retrieve(path, paramSupplier.get(), TrieMatchingMode.values()[mode++]); + } + }; } } diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index 04410662cc4..96b95491319 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -48,7 +48,6 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; @@ -153,9 +152,8 @@ public class RestController implements HttpServerTransport.Dispatcher { usageService.addRestHandler((BaseRestHandler) handler); } final RestHandler maybeWrappedHandler = handlerWrapper.apply(handler); - handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), (mHandlers, newMHandler) -> { - return mHandlers.addMethods(maybeWrappedHandler, method); - }); + handlers.insertOrUpdate(path, new MethodHandlers(path, maybeWrappedHandler, method), + (mHandlers, newMHandler) -> mHandlers.addMethods(maybeWrappedHandler, method)); } @Override @@ -198,22 +196,19 @@ public class RestController implements HttpServerTransport.Dispatcher { } } - /** - * Dispatch the request, if possible, returning true if a response was sent or false otherwise. - */ - private boolean dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception { + private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception { final int contentLength = request.content().length(); if (contentLength > 0) { final XContentType xContentType = request.getXContentType(); if (xContentType == null) { sendContentTypeErrorMessage(request.getAllHeaderValues("Content-Type"), channel); - return true; + return; + } + if (handler.supportsContentStream() && xContentType != XContentType.JSON && xContentType != XContentType.SMILE) { + channel.sendResponse(BytesRestResponse.createSimpleErrorResponse(channel, RestStatus.NOT_ACCEPTABLE, + "Content-Type [" + xContentType + "] does not support stream parsing. Use JSON or SMILE instead")); + return; } - if (handler.supportsContentStream() && xContentType != XContentType.JSON && xContentType != XContentType.SMILE) { - channel.sendResponse(BytesRestResponse.createSimpleErrorResponse(channel, RestStatus.NOT_ACCEPTABLE, - "Content-Type [" + xContentType + "] does not support stream parsing. Use JSON or SMILE instead")); - return true; - } } RestChannel responseChannel = channel; try { @@ -228,7 +223,6 @@ public class RestController implements HttpServerTransport.Dispatcher { } catch (Exception e) { responseChannel.sendResponse(new BytesRestResponse(responseChannel, e)); } - return true; } private boolean handleNoHandlerFound(String rawPath, RestRequest.Method method, String uri, RestChannel channel) { @@ -297,7 +291,8 @@ public class RestController implements HttpServerTransport.Dispatcher { if (handleNoHandlerFound(rawPath, requestMethod, uri, channel)) { return; } - } else if (dispatchRequest(request, channel, handler)) { + } else { + dispatchRequest(request, channel, handler); return; } } @@ -402,8 +397,11 @@ public class RestController implements HttpServerTransport.Dispatcher { private Set getValidHandlerMethodSet(String rawPath) { Set validMethods = new HashSet<>(); Iterator allHandlers = getAllHandlers(null, rawPath); - for (Iterator it = allHandlers; it.hasNext(); ) { - Optional.ofNullable(it.next()).map(mh -> validMethods.addAll(mh.getValidMethods())); + while (allHandlers.hasNext()) { + final MethodHandlers methodHandlers = allHandlers.next(); + if (methodHandlers != null) { + validMethods.addAll(methodHandlers.getValidMethods()); + } } return validMethods; }