From 714f7f17e364cf27869a17b4b3b67452937f30bb Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 3 Jun 2023 10:46:12 +0200 Subject: [PATCH 01/10] Made the ContextHandler graceful Improved semantics of Callbacks to facilitate correct nesting of callbacks --- .../deploy/providers/ContextProvider.java | 3 + .../jetty/server/handler/ContextHandler.java | 159 +++++++++++++---- .../jetty/server/handler/GracefulHandler.java | 4 +- .../server/handler/ContextHandlerTest.java | 166 ++++++++++++++++++ .../java/org/eclipse/jetty/util/Callback.java | 142 ++++++++------- 5 files changed, 370 insertions(+), 104 deletions(-) diff --git a/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java b/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java index 21f3e351ecf6..d0c020788919 100644 --- a/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java +++ b/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java @@ -317,6 +317,9 @@ else if (Supplier.class.isAssignableFrom(context.getClass())) if (context instanceof Deployable deployable) deployable.initializeDefaults(properties); + if (Boolean.valueOf(properties.get("graceful"))) + contextHandler.setGraceful(true); + return contextHandler; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 17dced5c2a56..36a48e0e7320 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -28,8 +28,10 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.atomic.LongAdder; import java.util.function.Consumer; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; @@ -65,12 +67,6 @@ public class ContextHandler extends Handler.Wrapper implements Attributes, Graceful, AliasCheck { - // TODO where should the alias checking go? - // TODO add protected paths to ServletContextHandler? - // TODO what about ObjectFactory stuff - // TODO what about a Context logger? - // TODO init param stuff to ServletContextHandler - private static final Logger LOG = LoggerFactory.getLogger(ContextHandler.class); private static final ThreadLocal __context = new ThreadLocal<>(); @@ -130,6 +126,8 @@ public static String getServerInfo() private final MimeTypes.Wrapper _mimeTypes = new MimeTypes.Wrapper(); private final List _contextListeners = new CopyOnWriteArrayList<>(); private final List _vhosts = new ArrayList<>(); + private final LongAdder _requests = new LongAdder(); + private CompletableFuture _shutdown; private String _displayName; private String _contextPath = "/"; @@ -143,11 +141,12 @@ public static String getServerInfo() private File _tempDirectory; private boolean _tempDirectoryPersisted = false; private boolean _tempDirectoryCreated = false; + private boolean _graceful; public enum Availability { STOPPED, // stopped and can't be made unavailable nor shutdown - STARTING, // starting inside of doStart. It may go to any of the next states. + STARTING, // starting inside doStart. It may go to any of the next states. AVAILABLE, // running normally UNAVAILABLE, // Either a startup error or explicit call to setAvailable(false) SHUTDOWN, // graceful shutdown @@ -199,6 +198,40 @@ protected ScopedContext newContext() return new ScopedContext(); } + /** + * @return true if {@link #shutdown()} will wait for all requests to complete. + */ + public boolean isGraceful() + { + return _graceful; + } + + /** + * Set if this context should gracefully shutdown by waiting for all current requests to + * complete. This can be set via the "graceful" property of the {@code DeploymentManager}. + * @see #shutdown() + * @see #getCurrentRequests() + * @param graceful true if {@link #shutdown()} will wait for all requests to complete. + */ + public void setGraceful(boolean graceful) + { + if (graceful && isStarted()) + throw new IllegalStateException(getState()); + _graceful = graceful; + + _requests.reset(); + if (!graceful && _shutdown != null) + _shutdown.complete(null); + } + + /** + * @return The number of current requests, or -1 if {@link #isGraceful()} is false. + */ + public long getCurrentRequests() + { + return _graceful ? _requests.sum() : -1; + } + /** * @return The temporary directory configured for the context, or null if none configured. * @see Context#getTempDirectory() @@ -450,11 +483,21 @@ public void setClassLoader(ClassLoader contextLoader) @ManagedAttribute("The file classpath") public String getClassPath() { - // TODO may need to handle one level of parent classloader for API ? if (_classLoader == null || !(_classLoader instanceof URLClassLoader loader)) return null; - String classpath = URIUtil.streamOf(loader) + Stream stream = URIUtil.streamOf(loader); + + // Add paths from any parent loader that is not the Server loader + ClassLoader parent = loader.getParent(); + while (parent != null && parent != Server.class.getClassLoader()) + { + if (parent instanceof URLClassLoader parentUrlLoader) + stream = Stream.concat(stream, URIUtil.streamOf(parentUrlLoader)); + parent = parent.getParent(); + } + + String classpath = stream .map(URI::toASCIIString) .collect(Collectors.joining(File.pathSeparator)); if (StringUtil.isBlank(classpath)) @@ -574,14 +617,10 @@ protected void notifyExitScope(Request request) } } - /** - * @return true if this context is shutting down - */ @ManagedAttribute("true for graceful shutdown, which allows existing requests to complete") public boolean isShutdown() { - // TODO - return false; + return _availability.get() == Availability.SHUTDOWN; } /** @@ -591,10 +630,34 @@ public boolean isShutdown() @Override public CompletableFuture shutdown() { - // TODO - CompletableFuture completableFuture = new CompletableFuture<>(); - completableFuture.complete(null); - return completableFuture; + while (true) + { + Availability availability = _availability.get(); + switch (availability) + { + case STOPPED -> + { + return CompletableFuture.completedFuture(null); + } + case SHUTDOWN -> + { + return _shutdown; + } + default -> + { + if (!_availability.compareAndSet(availability, Availability.SHUTDOWN)) + continue; + checkShutdown(); + return _shutdown; + } + } + } + } + + protected void checkShutdown() + { + if (_graceful && !_shutdown.isDone() && _requests.sum() == 0) + _shutdown.complete(null); } /** @@ -642,15 +705,16 @@ public void setAvailable(boolean available) Availability availability = _availability.get(); switch (availability) { - case STARTING: - case AVAILABLE: - if (!_availability.compareAndSet(availability, Availability.UNAVAILABLE)) - continue; - break; - default: - break; + case STARTING, AVAILABLE -> + { + if (_availability.compareAndSet(availability, Availability.UNAVAILABLE)) + return; + } + default -> + { + return; + } } - break; } } } @@ -661,6 +725,8 @@ protected void doStart() throws Exception if (getContextPath() == null) throw new IllegalStateException("Null contextPath"); + _shutdown = _graceful ? new CompletableFuture<>() : CompletableFuture.completedFuture(null); + _requests.reset(); _availability.set(Availability.STARTING); try { @@ -713,6 +779,8 @@ protected void createTempDirectory() @Override protected void doStop() throws Exception { + if (_shutdown != null && !_shutdown.isDone()) + _shutdown.complete(null); _context.call(super::doStop, null); File tempDirectory = getTempDirectory(); @@ -814,9 +882,15 @@ public boolean handle(Request request, Response response, Callback callback) thr // Past this point we are calling the downstream handler in scope. ClassLoader lastLoader = enterScope(contextRequest); ContextResponse contextResponse = wrapResponse(contextRequest, response); + if (_graceful) + callback = new CompletionCallback(callback); try { - return handler.handle(contextRequest, contextResponse, callback); + boolean handled = handler.handle(contextRequest, contextResponse, callback); + if (_graceful && !handled && callback instanceof CompletionCallback completionCallback) + completionCallback.completed(); + + return handled; } catch (Throwable t) { @@ -828,6 +902,8 @@ public boolean handle(Request request, Response response, Callback callback) thr // We exit scope here, even though handle() is asynchronous, // as we have wrapped all our callbacks to re-enter the scope. exitScope(contextRequest, request.getContext(), lastLoader); + if (_graceful && isShutdown()) + checkShutdown(); } } @@ -1099,6 +1175,8 @@ public String toString() b.append(getContextPath()); b.append(",b=").append(getBaseResource()); b.append(",a=").append(_availability.get()); + if (_graceful) + b.append(",r=").append(_requests.sum()); if (!vhosts.isEmpty()) { @@ -1119,7 +1197,7 @@ public String toString() private String normalizeHostname(String host) { - // TODO is this needed? if so, should be it somewhere eles? + // TODO is this needed? if so, should be it somewhere else? if (host == null) return null; int connectorIndex = host.indexOf('@'); @@ -1151,14 +1229,6 @@ public H getContextHandler() return (H)ContextHandler.this; } - @Override - public Object getAttribute(String name) - { - // TODO the Attributes.Layer is a little different to previous - // behaviour. We need to verify if that is OK - return super.getAttribute(name); - } - @Override public Request.Handler getErrorHandler() { @@ -1368,4 +1438,21 @@ public String toString() '}'; } } + + private class CompletionCallback extends Callback.Nested + { + public CompletionCallback(Callback callback) + { + super(callback); + _requests.increment(); + } + + @Override + public void completed() + { + _requests.decrement(); + if (isShutdown()) + checkShutdown(); + } + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java index 5719d19663cd..d1010d61646e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java @@ -91,8 +91,8 @@ public boolean handle(Request request, Response response, Callback callback) thr } catch (Throwable t) { - shutdownCallback.decrement(); - throw t; + Response.writeError(request, response, shutdownCallback, t); + return true; } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java index 0d82345b3c7f..6892d30d21ee 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java @@ -19,20 +19,27 @@ import java.net.URL; import java.net.URLClassLoader; import java.nio.ByteBuffer; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Stream; +import org.awaitility.Awaitility; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.server.Connector; @@ -40,6 +47,7 @@ import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.HttpStream; +import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.MockConnectionMetaData; import org.eclipse.jetty.server.MockConnector; import org.eclipse.jetty.server.MockHttpStream; @@ -891,4 +899,162 @@ void assertInContext(Context context, Request request) assertThat(r, sameInstance(request)); } } + + @Test + public void testGraceful() throws Exception + { + CountDownLatch latch0 = new CountDownLatch(1); + CountDownLatch latch1 = new CountDownLatch(1); + + CountDownLatch requests = new CountDownLatch(7); + + Handler handler = new AbstractHandler() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + requests.countDown(); + switch (request.getContext().getPathInContext(request.getHttpURI().getCanonicalPath())) + { + case "/ignore0" -> + { + try + { + latch0.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + return false; + } + + case "/ignore1" -> + { + try + { + latch1.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + return false; + } + + case "/ok0" -> + { + try + { + latch0.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + } + + case "/ok1" -> + { + try + { + latch1.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + } + + case "/fail0" -> + { + try + { + latch0.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + throw new QuietException.Exception("expected0"); + } + + case "/fail1" -> + { + try + { + latch1.await(); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + callback.failed(new QuietException.Exception("expected1")); + } + + default -> + { + } + } + + response.setStatus(HttpStatus.OK_200); + callback.succeeded(); + return true; + } + }; + _contextHandler.setHandler(handler); + _contextHandler.setGraceful(true); + LocalConnector connector = new LocalConnector(_server); + _server.addConnector(connector); + _server.start(); + + HttpTester.Response response = HttpTester.parseResponse(connector.getResponse("GET /ctx/ HTTP/1.0\r\n\r\n")); + assertThat(response.getStatus(), is(HttpStatus.OK_200)); + + List endPoints = new ArrayList<>(); + for (String target : new String[] {"/ignore", "/ok", "/fail"}) + { + for (int batch = 0; batch <= 1; batch++) + { + LocalConnector.LocalEndPoint endPoint = connector.executeRequest("GET /ctx%s%d HTTP/1.0\r\n\r\n".formatted(target, batch)); + endPoints.add(endPoint); + } + } + + assertTrue(requests.await(10, TimeUnit.SECONDS)); + assertThat(_contextHandler.getCurrentRequests(), is(6L)); + + CompletableFuture shutdown = _contextHandler.shutdown(); + assertFalse(shutdown.isDone()); + assertThat(_contextHandler.getCurrentRequests(), is(6L)); + + response = HttpTester.parseResponse(connector.getResponse("GET /ctx/ HTTP/1.0\r\n\r\n")); + assertThat(response.getStatus(), is(HttpStatus.SERVICE_UNAVAILABLE_503)); + + latch0.countDown(); + + response = HttpTester.parseResponse(endPoints.get(0).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.NOT_FOUND_404)); + response = HttpTester.parseResponse(endPoints.get(2).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.OK_200)); + response = HttpTester.parseResponse(endPoints.get(4).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.INTERNAL_SERVER_ERROR_500)); + + assertFalse(shutdown.isDone()); + Awaitility.waitAtMost(10, TimeUnit.SECONDS).until(() -> _contextHandler.getCurrentRequests() == 3L); + assertThat(_contextHandler.getCurrentRequests(), is(3L)); + + latch1.countDown(); + + response = HttpTester.parseResponse(endPoints.get(1).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.NOT_FOUND_404)); + response = HttpTester.parseResponse(endPoints.get(3).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.OK_200)); + response = HttpTester.parseResponse(endPoints.get(5).getResponse()); + assertThat(response.getStatus(), is(HttpStatus.INTERNAL_SERVER_ERROR_500)); + + shutdown.get(10, TimeUnit.SECONDS); + assertTrue(shutdown.isDone()); + assertThat(_contextHandler.getCurrentRequests(), is(0L)); + } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index 2c91399e1e4f..1f2426ba2ba3 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -14,6 +14,8 @@ package org.eclipse.jetty.util; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.eclipse.jetty.util.thread.Invocable; @@ -155,16 +157,20 @@ static Callback from(InvocationType invocationType, Runnable success, ConsumerA Callback implementation that calls the {@link #completed()} method when it either succeeds or fails.

*/ - class Completing implements Callback + interface Completing extends Callback { - private final InvocationType invocationType; - - public Completing() - { - this(InvocationType.BLOCKING); - } - - public Completing(InvocationType invocationType) - { - this.invocationType = invocationType; - } - - @Override - public void succeeded() - { - completed(); - } - - @Override - public void failed(Throwable x) - { - completed(); - } - - @Override - public InvocationType getInvocationType() - { - return invocationType; - } - - public void completed() - { - } + void completed(); } /** * Nested Completing Callback that completes after * completing the nested callback */ - class Nested extends Completing + class Nested implements Completing { - private final Callback callback; + private final AtomicReference _callback; public Nested(Callback callback) { - super(Invocable.getInvocationType(callback)); - this.callback = callback; + _callback = new AtomicReference<>(callback); } - public Nested(Nested nested) + public Callback getCallback() { - this(nested.callback); + return _callback.get(); } - public Callback getCallback() + @Override + public void completed() { - return callback; } @Override public void succeeded() { - try - { - callback.succeeded(); - } - finally + Callback callback = _callback.getAndSet(null); + if (callback != null) { - completed(); + try + { + callback.succeeded(); + } + finally + { + completed(); + } } } @Override public void failed(Throwable x) { - try + Callback callback = _callback.getAndSet(null); + if (callback != null) { - callback.failed(x); - } - finally - { - completed(); + try + { + callback.failed(x); + } + finally + { + completed(); + } } } @Override public InvocationType getInvocationType() { - return callback.getInvocationType(); + Callback callback = _callback.get(); + return callback == null ? InvocationType.NON_BLOCKING : callback.getInvocationType(); } @Override public String toString() { - return "%s@%x:%s".formatted(getClass().getSimpleName(), hashCode(), callback); + return "%s@%x:%s".formatted(getClass().getSimpleName(), hashCode(), _callback.get()); } } From 2cd3095c8cf739fbf4856e31665f9270ed1afe8b Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 3 Jun 2023 21:08:31 +0200 Subject: [PATCH 02/10] Graceful context --- .../jetty/server/handler/GracefulHandler.java | 16 +--- .../java/org/eclipse/jetty/util/Callback.java | 82 +++++++------------ 2 files changed, 32 insertions(+), 66 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java index d1010d61646e..09255b17c5a7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java @@ -86,7 +86,7 @@ public boolean handle(Request request, Response response, Callback callback) thr { boolean handled = super.handle(request, response, shutdownCallback); if (!handled) - shutdownCallback.decrement(); + shutdownCallback.completed(); return handled; } catch (Throwable t) @@ -128,19 +128,9 @@ public void decrement() } @Override - public void failed(Throwable x) + public void completed() { - decrement(); - super.failed(x); - if (isShutdown()) - shutdown.check(); - } - - @Override - public void succeeded() - { - decrement(); - super.succeeded(); + dispatchedStats.decrement(); if (isShutdown()) shutdown.check(); } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index 1f2426ba2ba3..f6d43c2ac23c 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -278,32 +278,40 @@ static Callback from(Runnable completed, Callback callback) { return new Callback() { + private final AtomicBoolean _completed = new AtomicBoolean(); + @Override public void succeeded() { - try - { - completed.run(); - callback.succeeded(); - } - catch (Throwable t) + if (_completed.compareAndSet(false, true)) { - callback.failed(t); + try + { + completed.run(); + callback.succeeded(); + } + catch (Throwable t) + { + callback.failed(t); + } } } @Override public void failed(Throwable x) { - try - { - completed.run(); - } - catch (Throwable t) + if (_completed.compareAndSet(false, true)) { - x.addSuppressed(t); + try + { + completed.run(); + } + catch (Throwable t) + { + x.addSuppressed(t); + } + callback.failed(x); } - callback.failed(x); } }; } @@ -358,6 +366,12 @@ public void failed(Throwable x) callback1.failed(x); callback2.failed(x); } + + @Override + public InvocationType getInvocationType() + { + return Invocable.combine(Invocable.getInvocationType(callback1), Invocable.getInvocationType(callback2)); + } }; } @@ -447,45 +461,7 @@ static Callback combine(Callback cb1, Callback cb2) if (cb2 == null) return cb1; - return new Callback() - { - @Override - public void succeeded() - { - try - { - cb1.succeeded(); - } - finally - { - cb2.succeeded(); - } - } - - @Override - public void failed(Throwable x) - { - try - { - cb1.failed(x); - } - catch (Throwable t) - { - if (x != t) - x.addSuppressed(t); - } - finally - { - cb2.failed(x); - } - } - - @Override - public InvocationType getInvocationType() - { - return Invocable.combine(Invocable.getInvocationType(cb1), Invocable.getInvocationType(cb2)); - } - }; + return from(cb1, cb2); } /** From 517313dac584b0069a767881fe84e3b84443ba6b Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 4 Jun 2023 13:24:12 +0200 Subject: [PATCH 03/10] removed unrelated change --- .../jetty/server/handler/ContextHandler.java | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 36a48e0e7320..2ef1ad94294b 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -31,7 +31,6 @@ import java.util.concurrent.atomic.LongAdder; import java.util.function.Consumer; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; @@ -483,21 +482,11 @@ public void setClassLoader(ClassLoader contextLoader) @ManagedAttribute("The file classpath") public String getClassPath() { + // TODO may need to handle one level of parent classloader for API ? if (_classLoader == null || !(_classLoader instanceof URLClassLoader loader)) return null; - Stream stream = URIUtil.streamOf(loader); - - // Add paths from any parent loader that is not the Server loader - ClassLoader parent = loader.getParent(); - while (parent != null && parent != Server.class.getClassLoader()) - { - if (parent instanceof URLClassLoader parentUrlLoader) - stream = Stream.concat(stream, URIUtil.streamOf(parentUrlLoader)); - parent = parent.getParent(); - } - - String classpath = stream + String classpath = URIUtil.streamOf(loader) .map(URI::toASCIIString) .collect(Collectors.joining(File.pathSeparator)); if (StringUtil.isBlank(classpath)) From f5afbf88e2cc44c9b8ea11d2caffcc8c0ff4df38 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 4 Jun 2023 13:49:53 +0200 Subject: [PATCH 04/10] revert Signed-off-by: gregw --- .../java/org/eclipse/jetty/server/handler/ContextHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 2ef1ad94294b..8228a3b940d1 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1186,7 +1186,7 @@ public String toString() private String normalizeHostname(String host) { - // TODO is this needed? if so, should be it somewhere else? + // TODO is this needed? if so, should be it somewhere eles? if (host == null) return null; int connectorIndex = host.indexOf('@'); From 191968a00affa875fc2f140f2871512ebef42bc1 Mon Sep 17 00:00:00 2001 From: gregw Date: Sun, 4 Jun 2023 14:47:46 +0200 Subject: [PATCH 05/10] removed unrelated change --- .../src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java index 3d0055b82af7..77b06cf0c64d 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java @@ -1530,7 +1530,7 @@ public void failed(final Throwable x) if (x instanceof HttpException httpException) { MetaData.Response responseMeta = new MetaData.Response(httpException.getCode(), httpException.getReason(), HttpVersion.HTTP_1_1, HttpFields.build().add(HttpFields.CONNECTION_CLOSE), 0); - send(_request.getMetaData(), responseMeta, null, true, new Nested(this) + send(_request.getMetaData(), responseMeta, null, true, new Nested(this.getCallback()) { @Override public void succeeded() From a765f910a8609a22cb5ef208a4851c6875c16a60 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 5 Jun 2023 16:57:31 +0200 Subject: [PATCH 06/10] Updates after review Reverted most changes to Callback other than cleanups Removed all shutdown mechanisms from ContextHandler --- .../deploy/providers/ContextProvider.java | 3 - .../jetty/server/handler/ContextHandler.java | 119 +----------------- .../jetty/server/handler/GracefulHandler.java | 37 +++--- .../jetty/server/GracefulHandlerTest.java | 3 +- .../server/handler/ContextHandlerTest.java | 20 +-- .../java/org/eclipse/jetty/util/Callback.java | 102 ++++++--------- 6 files changed, 73 insertions(+), 211 deletions(-) diff --git a/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java b/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java index d0c020788919..21f3e351ecf6 100644 --- a/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java +++ b/jetty-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ContextProvider.java @@ -317,9 +317,6 @@ else if (Supplier.class.isAssignableFrom(context.getClass())) if (context instanceof Deployable deployable) deployable.initializeDefaults(properties); - if (Boolean.valueOf(properties.get("graceful"))) - contextHandler.setGraceful(true); - return contextHandler; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 8228a3b940d1..9e04372aa8a3 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -25,10 +25,8 @@ import java.util.List; import java.util.Objects; import java.util.Set; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.atomic.LongAdder; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -55,7 +53,6 @@ import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.component.ClassLoaderDump; import org.eclipse.jetty.util.component.Dumpable; -import org.eclipse.jetty.util.component.Graceful; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.resource.Resource; import org.eclipse.jetty.util.resource.ResourceFactory; @@ -64,7 +61,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class ContextHandler extends Handler.Wrapper implements Attributes, Graceful, AliasCheck +public class ContextHandler extends Handler.Wrapper implements Attributes, AliasCheck { private static final Logger LOG = LoggerFactory.getLogger(ContextHandler.class); private static final ThreadLocal __context = new ThreadLocal<>(); @@ -125,8 +122,6 @@ public static String getServerInfo() private final MimeTypes.Wrapper _mimeTypes = new MimeTypes.Wrapper(); private final List _contextListeners = new CopyOnWriteArrayList<>(); private final List _vhosts = new ArrayList<>(); - private final LongAdder _requests = new LongAdder(); - private CompletableFuture _shutdown; private String _displayName; private String _contextPath = "/"; @@ -140,7 +135,6 @@ public static String getServerInfo() private File _tempDirectory; private boolean _tempDirectoryPersisted = false; private boolean _tempDirectoryCreated = false; - private boolean _graceful; public enum Availability { @@ -148,7 +142,6 @@ public enum Availability STARTING, // starting inside doStart. It may go to any of the next states. AVAILABLE, // running normally UNAVAILABLE, // Either a startup error or explicit call to setAvailable(false) - SHUTDOWN, // graceful shutdown } /** @@ -197,40 +190,6 @@ protected ScopedContext newContext() return new ScopedContext(); } - /** - * @return true if {@link #shutdown()} will wait for all requests to complete. - */ - public boolean isGraceful() - { - return _graceful; - } - - /** - * Set if this context should gracefully shutdown by waiting for all current requests to - * complete. This can be set via the "graceful" property of the {@code DeploymentManager}. - * @see #shutdown() - * @see #getCurrentRequests() - * @param graceful true if {@link #shutdown()} will wait for all requests to complete. - */ - public void setGraceful(boolean graceful) - { - if (graceful && isStarted()) - throw new IllegalStateException(getState()); - _graceful = graceful; - - _requests.reset(); - if (!graceful && _shutdown != null) - _shutdown.complete(null); - } - - /** - * @return The number of current requests, or -1 if {@link #isGraceful()} is false. - */ - public long getCurrentRequests() - { - return _graceful ? _requests.sum() : -1; - } - /** * @return The temporary directory configured for the context, or null if none configured. * @see Context#getTempDirectory() @@ -606,49 +565,6 @@ protected void notifyExitScope(Request request) } } - @ManagedAttribute("true for graceful shutdown, which allows existing requests to complete") - public boolean isShutdown() - { - return _availability.get() == Availability.SHUTDOWN; - } - - /** - * Set shutdown status. This field allows for graceful shutdown of a context. A started context may be put into non accepting state so that existing - * requests can complete, but no new requests are accepted. - */ - @Override - public CompletableFuture shutdown() - { - while (true) - { - Availability availability = _availability.get(); - switch (availability) - { - case STOPPED -> - { - return CompletableFuture.completedFuture(null); - } - case SHUTDOWN -> - { - return _shutdown; - } - default -> - { - if (!_availability.compareAndSet(availability, Availability.SHUTDOWN)) - continue; - checkShutdown(); - return _shutdown; - } - } - } - } - - protected void checkShutdown() - { - if (_graceful && !_shutdown.isDone() && _requests.sum() == 0) - _shutdown.complete(null); - } - /** * @return false if this context is unavailable (sends 503) */ @@ -714,8 +630,6 @@ protected void doStart() throws Exception if (getContextPath() == null) throw new IllegalStateException("Null contextPath"); - _shutdown = _graceful ? new CompletableFuture<>() : CompletableFuture.completedFuture(null); - _requests.reset(); _availability.set(Availability.STARTING); try { @@ -768,8 +682,6 @@ protected void createTempDirectory() @Override protected void doStop() throws Exception { - if (_shutdown != null && !_shutdown.isDone()) - _shutdown.complete(null); _context.call(super::doStop, null); File tempDirectory = getTempDirectory(); @@ -871,15 +783,9 @@ public boolean handle(Request request, Response response, Callback callback) thr // Past this point we are calling the downstream handler in scope. ClassLoader lastLoader = enterScope(contextRequest); ContextResponse contextResponse = wrapResponse(contextRequest, response); - if (_graceful) - callback = new CompletionCallback(callback); try { - boolean handled = handler.handle(contextRequest, contextResponse, callback); - if (_graceful && !handled && callback instanceof CompletionCallback completionCallback) - completionCallback.completed(); - - return handled; + return handler.handle(contextRequest, contextResponse, callback); } catch (Throwable t) { @@ -891,8 +797,6 @@ public boolean handle(Request request, Response response, Callback callback) thr // We exit scope here, even though handle() is asynchronous, // as we have wrapped all our callbacks to re-enter the scope. exitScope(contextRequest, request.getContext(), lastLoader); - if (_graceful && isShutdown()) - checkShutdown(); } } @@ -1164,8 +1068,6 @@ public String toString() b.append(getContextPath()); b.append(",b=").append(getBaseResource()); b.append(",a=").append(_availability.get()); - if (_graceful) - b.append(",r=").append(_requests.sum()); if (!vhosts.isEmpty()) { @@ -1427,21 +1329,4 @@ public String toString() '}'; } } - - private class CompletionCallback extends Callback.Nested - { - public CompletionCallback(Callback callback) - { - super(callback); - _requests.increment(); - } - - @Override - public void completed() - { - _requests.decrement(); - if (isShutdown()) - checkShutdown(); - } - } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java index 09255b17c5a7..8dd5ae16968e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java @@ -21,6 +21,8 @@ import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.CountingCallback; +import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.component.Graceful; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -32,17 +34,17 @@ public class GracefulHandler extends Handler.Wrapper implements Graceful { private static final Logger LOG = LoggerFactory.getLogger(GracefulHandler.class); - private final LongAdder dispatchedStats = new LongAdder(); - private final Shutdown shutdown; + private final LongAdder _requests = new LongAdder(); + private final Shutdown _shutdown; public GracefulHandler() { - shutdown = new Shutdown(this) + _shutdown = new Shutdown(this) { @Override public boolean isShutdownDone() { - long count = dispatchedStats.sum(); + long count = getCurrentRequests(); if (LOG.isDebugEnabled()) LOG.debug("isShutdownDone: count {}", count); return count == 0; @@ -50,6 +52,12 @@ public boolean isShutdownDone() }; } + @ManagedAttribute("current requests") + public long getCurrentRequests() + { + return _requests.sum(); + } + /** * Flag indicating that Graceful shutdown has been initiated. * @@ -59,7 +67,7 @@ public boolean isShutdownDone() @Override public boolean isShutdown() { - return shutdown.isShutdown(); + return _shutdown.isShutdown(); } @Override @@ -97,7 +105,7 @@ public boolean handle(Request request, Response response, Callback callback) thr finally { if (isShutdown()) - shutdown.check(); + _shutdown.check(); } } @@ -106,33 +114,28 @@ public CompletableFuture shutdown() { if (LOG.isDebugEnabled()) LOG.debug("Shutdown requested"); - return shutdown.shutdown(); + return _shutdown.shutdown(); } - private class ShutdownTrackingCallback extends Callback.Nested + private class ShutdownTrackingCallback extends CountingCallback { final Request request; final Response response; public ShutdownTrackingCallback(Request request, Response response, Callback callback) { - super(callback); + super(callback, 1); this.request = request; this.response = response; - dispatchedStats.increment(); - } - - public void decrement() - { - dispatchedStats.decrement(); + _requests.increment(); } @Override public void completed() { - dispatchedStats.decrement(); + _requests.decrement(); if (isShutdown()) - shutdown.check(); + _shutdown.check(); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulHandlerTest.java index 3b0e62af577c..85cf73430aa1 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/GracefulHandlerTest.java @@ -52,12 +52,11 @@ public class GracefulHandlerTest { private static final Logger LOG = LoggerFactory.getLogger(GracefulHandlerTest.class); private Server server; - private ServerConnector connector; public Server createServer(Handler handler) throws Exception { server = new Server(); - connector = new ServerConnector(server, 1, 1); + ServerConnector connector = new ServerConnector(server, 1, 1); connector.setIdleTimeout(10000); connector.setShutdownIdleTimeout(1000); connector.setPort(0); diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java index 6892d30d21ee..5e7f9d74aeaf 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java @@ -60,6 +60,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.component.Graceful; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -903,12 +904,14 @@ void assertInContext(Context context, Request request) @Test public void testGraceful() throws Exception { + // This is really just another test of GracefulHandler, but good to check it works inside of ContextHandler + CountDownLatch latch0 = new CountDownLatch(1); CountDownLatch latch1 = new CountDownLatch(1); CountDownLatch requests = new CountDownLatch(7); - Handler handler = new AbstractHandler() + Handler handler = new Handler.Abstract() { @Override public boolean handle(Request request, Response response, Callback callback) throws Exception @@ -1003,7 +1006,8 @@ public boolean handle(Request request, Response response, Callback callback) thr } }; _contextHandler.setHandler(handler); - _contextHandler.setGraceful(true); + GracefulHandler gracefulHandler = new GracefulHandler(); + _contextHandler.insertHandler(gracefulHandler); LocalConnector connector = new LocalConnector(_server); _server.addConnector(connector); _server.start(); @@ -1022,11 +1026,11 @@ public boolean handle(Request request, Response response, Callback callback) thr } assertTrue(requests.await(10, TimeUnit.SECONDS)); - assertThat(_contextHandler.getCurrentRequests(), is(6L)); + assertThat(gracefulHandler.getCurrentRequests(), is(6L)); - CompletableFuture shutdown = _contextHandler.shutdown(); + CompletableFuture shutdown = Graceful.shutdown(_contextHandler); assertFalse(shutdown.isDone()); - assertThat(_contextHandler.getCurrentRequests(), is(6L)); + assertThat(gracefulHandler.getCurrentRequests(), is(6L)); response = HttpTester.parseResponse(connector.getResponse("GET /ctx/ HTTP/1.0\r\n\r\n")); assertThat(response.getStatus(), is(HttpStatus.SERVICE_UNAVAILABLE_503)); @@ -1041,8 +1045,8 @@ public boolean handle(Request request, Response response, Callback callback) thr assertThat(response.getStatus(), is(HttpStatus.INTERNAL_SERVER_ERROR_500)); assertFalse(shutdown.isDone()); - Awaitility.waitAtMost(10, TimeUnit.SECONDS).until(() -> _contextHandler.getCurrentRequests() == 3L); - assertThat(_contextHandler.getCurrentRequests(), is(3L)); + Awaitility.waitAtMost(10, TimeUnit.SECONDS).until(() -> gracefulHandler.getCurrentRequests() == 3L); + assertThat(gracefulHandler.getCurrentRequests(), is(3L)); latch1.countDown(); @@ -1055,6 +1059,6 @@ public boolean handle(Request request, Response response, Callback callback) thr shutdown.get(10, TimeUnit.SECONDS); assertTrue(shutdown.isDone()); - assertThat(_contextHandler.getCurrentRequests(), is(0L)); + assertThat(gracefulHandler.getCurrentRequests(), is(0L)); } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index f6d43c2ac23c..9a74c2d2374f 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -13,9 +13,8 @@ package org.eclipse.jetty.util; +import java.util.Objects; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.eclipse.jetty.util.thread.Invocable; @@ -157,20 +156,16 @@ static Callback from(InvocationType invocationType, Runnable success, Consumer _callback; + private final Callback callback; public Nested(Callback callback) { - _callback = new AtomicReference<>(callback); + this.callback = Objects.requireNonNull(callback); } public Callback getCallback() { - return _callback.get(); + return callback; } @Override @@ -409,48 +392,39 @@ public void completed() @Override public void succeeded() { - Callback callback = _callback.getAndSet(null); - if (callback != null) + try { - try - { - callback.succeeded(); - } - finally - { - completed(); - } + callback.succeeded(); + } + finally + { + completed(); } } @Override public void failed(Throwable x) { - Callback callback = _callback.getAndSet(null); - if (callback != null) + try { - try - { - callback.failed(x); - } - finally - { - completed(); - } + callback.failed(x); + } + finally + { + completed(); } } @Override public InvocationType getInvocationType() { - Callback callback = _callback.get(); - return callback == null ? InvocationType.NON_BLOCKING : callback.getInvocationType(); + return callback.getInvocationType(); } @Override public String toString() { - return "%s@%x:%s".formatted(getClass().getSimpleName(), hashCode(), _callback.get()); + return "%s@%x:%s".formatted(getClass().getSimpleName(), hashCode(), callback); } } From 256af9840c0588edfa7c2ff65dca9ce013873566 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 5 Jun 2023 17:02:05 +0200 Subject: [PATCH 07/10] Updates after review Reverted most changes to Callback other than cleanups Removed all shutdown mechanisms from ContextHandler --- .../ee10/servlet/ServletContextHandler.java | 3 +-- .../jetty/ee9/nested/ContextHandler.java | 23 +------------------ 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index 065aee4e7515..086b91af6952 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -95,7 +95,6 @@ import org.eclipse.jetty.util.component.Dumpable; import org.eclipse.jetty.util.component.DumpableCollection; import org.eclipse.jetty.util.component.Environment; -import org.eclipse.jetty.util.component.Graceful; import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.resource.Resource; import org.eclipse.jetty.util.resource.ResourceFactory; @@ -120,7 +119,7 @@ * cause confusion with {@link ServletContext}. */ @ManagedObject("Servlet Context Handler") -public class ServletContextHandler extends ContextHandler implements Graceful +public class ServletContextHandler extends ContextHandler { private static final Logger LOG = LoggerFactory.getLogger(ServletContextHandler.class); public static final Environment __environment = Environment.ensure("ee10"); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java index daedccd41975..6d855dd40b66 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java @@ -31,7 +31,6 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Supplier; @@ -90,7 +89,6 @@ import org.eclipse.jetty.util.annotation.ManagedObject; import org.eclipse.jetty.util.component.DumpableCollection; import org.eclipse.jetty.util.component.Environment; -import org.eclipse.jetty.util.component.Graceful; import org.eclipse.jetty.util.resource.Resource; import org.eclipse.jetty.util.resource.ResourceFactory; import org.eclipse.jetty.util.resource.Resources; @@ -121,7 +119,7 @@ *

*/ @ManagedObject("EE9 Context") -public class ContextHandler extends ScopedHandler implements Attributes, Graceful, Supplier +public class ContextHandler extends ScopedHandler implements Attributes, Supplier { public static final Environment ENVIRONMENT = Environment.ensure("ee9"); public static final int SERVLET_MAJOR_VERSION = 5; @@ -552,25 +550,6 @@ public boolean isDurableListener(EventListener listener) return getEventListeners().contains(listener); } - /** - * @return true if this context is shutting down - */ - @ManagedAttribute("true for graceful shutdown, which allows existing requests to complete") - public boolean isShutdown() - { - return _coreContextHandler.isShutdown(); - } - - /** - * Set shutdown status. This field allows for graceful shutdown of a context. A started context may be put into non accepting state so that existing - * requests can complete, but no new requests are accepted. - */ - @Override - public CompletableFuture shutdown() - { - return _coreContextHandler.shutdown(); - } - /** * @return false if this context is unavailable (sends 503) */ From 2e48f3e2ad2d5443e373d8b60dd79e7f60d39f2f Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 6 Jun 2023 15:20:17 +0200 Subject: [PATCH 08/10] Add GracefulShutdownTest plus tiny modifications to the API and javadoc Signed-off-by: Ludovic Orban --- .../jetty/util/component/Graceful.java | 5 ++ .../util/component/GracefulShutdownTest.java | 88 +++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/component/GracefulShutdownTest.java diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/Graceful.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/Graceful.java index 2b2456ffb253..8c0d9f62099c 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/Graceful.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/Graceful.java @@ -108,6 +108,11 @@ public void check() done.complete(null); } + /** + * This method can be called after {@link #shutdown()} has been called, but before + * {@link #check()} has been called with {@link #isShutdownDone()} having returned + * true to cancel the effects of the {@link #shutdown()} call. + */ public void cancel() { CompletableFuture done = _done.get(); diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/component/GracefulShutdownTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/component/GracefulShutdownTest.java new file mode 100644 index 000000000000..32adec61b5d5 --- /dev/null +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/component/GracefulShutdownTest.java @@ -0,0 +1,88 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.util.component; + +import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class GracefulShutdownTest +{ + @Test + public void testGracefulShutdown() throws Exception + { + AtomicBoolean isShutdown = new AtomicBoolean(); + Graceful.Shutdown shutdown = new Graceful.Shutdown("testGracefulShutdown") + { + @Override + public boolean isShutdownDone() + { + return isShutdown.get(); + } + }; + + assertThat(shutdown.isShutdown(), is(false)); + + shutdown.check(); + assertThat(shutdown.isShutdown(), is(false)); + + CompletableFuture cf = shutdown.shutdown(); + assertThat(shutdown.isShutdown(), is(true)); + shutdown.check(); + assertThat(shutdown.isShutdown(), is(true)); + + assertThrows(TimeoutException.class, () -> cf.get(10, TimeUnit.MILLISECONDS)); + + isShutdown.set(true); + shutdown.check(); + assertThat(shutdown.isShutdown(), is(true)); + assertThat(cf.get(), nullValue()); + } + + @Test + public void testGracefulShutdownCancel() throws Exception + { + AtomicBoolean isShutdown = new AtomicBoolean(); + Graceful.Shutdown shutdown = new Graceful.Shutdown("testGracefulShutdownCancel") + { + @Override + public boolean isShutdownDone() + { + return isShutdown.get(); + } + }; + + CompletableFuture cf1 = shutdown.shutdown(); + shutdown.cancel(); + assertThat(shutdown.isShutdown(), is(false)); + assertThrows(CancellationException.class, cf1::get); + + CompletableFuture cf2 = shutdown.shutdown(); + assertThat(shutdown.isShutdown(), is(true)); + isShutdown.set(true); + assertThrows(TimeoutException.class, () -> cf2.get(10, TimeUnit.MILLISECONDS)); + shutdown.check(); + assertThat(shutdown.isShutdown(), is(true)); + assertThat(cf2.get(), nullValue()); + } +} From e6825886eff892c3dbe066264372f2202c87e2c7 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 6 Jun 2023 23:12:04 +0200 Subject: [PATCH 09/10] updates from review --- .../src/main/java/org/eclipse/jetty/util/Callback.java | 2 +- .../src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index 9a74c2d2374f..afb1ff34d89c 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -232,7 +232,7 @@ public InvocationType getInvocationType() @Override public String toString() { - return "Callback@%x{%s,%s}".formatted(hashCode(), invocationType, completed); + return "Callback.Completing@%x{%s,%s}".formatted(hashCode(), invocationType, completed); } }; } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java index 77b06cf0c64d..cdbabc3c7265 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java @@ -1530,7 +1530,7 @@ public void failed(final Throwable x) if (x instanceof HttpException httpException) { MetaData.Response responseMeta = new MetaData.Response(httpException.getCode(), httpException.getReason(), HttpVersion.HTTP_1_1, HttpFields.build().add(HttpFields.CONNECTION_CLOSE), 0); - send(_request.getMetaData(), responseMeta, null, true, new Nested(this.getCallback()) + send(_request.getMetaData(), responseMeta, null, true, new Nested(getCallback()) { @Override public void succeeded() From 4683af20423c86f9512a7c402c31f4cf3d35f6f2 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 7 Jun 2023 15:51:35 +0200 Subject: [PATCH 10/10] Updates from review --- .../jetty/server/handler/GracefulHandler.java | 6 +- .../server/handler/ContextHandlerTest.java | 10 +-- .../java/org/eclipse/jetty/util/Callback.java | 87 +++++++++++-------- 3 files changed, 60 insertions(+), 43 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java index 8dd5ae16968e..99565a435db4 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GracefulHandler.java @@ -44,7 +44,7 @@ public GracefulHandler() @Override public boolean isShutdownDone() { - long count = getCurrentRequests(); + long count = getCurrentRequestCount(); if (LOG.isDebugEnabled()) LOG.debug("isShutdownDone: count {}", count); return count == 0; @@ -52,8 +52,8 @@ public boolean isShutdownDone() }; } - @ManagedAttribute("current requests") - public long getCurrentRequests() + @ManagedAttribute("number of requests being currently handled") + public long getCurrentRequestCount() { return _requests.sum(); } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java index 5e7f9d74aeaf..4f5c0b4300f5 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java @@ -1026,11 +1026,11 @@ public boolean handle(Request request, Response response, Callback callback) thr } assertTrue(requests.await(10, TimeUnit.SECONDS)); - assertThat(gracefulHandler.getCurrentRequests(), is(6L)); + assertThat(gracefulHandler.getCurrentRequestCount(), is(6L)); CompletableFuture shutdown = Graceful.shutdown(_contextHandler); assertFalse(shutdown.isDone()); - assertThat(gracefulHandler.getCurrentRequests(), is(6L)); + assertThat(gracefulHandler.getCurrentRequestCount(), is(6L)); response = HttpTester.parseResponse(connector.getResponse("GET /ctx/ HTTP/1.0\r\n\r\n")); assertThat(response.getStatus(), is(HttpStatus.SERVICE_UNAVAILABLE_503)); @@ -1045,8 +1045,8 @@ public boolean handle(Request request, Response response, Callback callback) thr assertThat(response.getStatus(), is(HttpStatus.INTERNAL_SERVER_ERROR_500)); assertFalse(shutdown.isDone()); - Awaitility.waitAtMost(10, TimeUnit.SECONDS).until(() -> gracefulHandler.getCurrentRequests() == 3L); - assertThat(gracefulHandler.getCurrentRequests(), is(3L)); + Awaitility.waitAtMost(10, TimeUnit.SECONDS).until(() -> gracefulHandler.getCurrentRequestCount() == 3L); + assertThat(gracefulHandler.getCurrentRequestCount(), is(3L)); latch1.countDown(); @@ -1059,6 +1059,6 @@ public boolean handle(Request request, Response response, Callback callback) thr shutdown.get(10, TimeUnit.SECONDS); assertTrue(shutdown.isDone()); - assertThat(gracefulHandler.getCurrentRequests(), is(0L)); + assertThat(gracefulHandler.getCurrentRequestCount(), is(0L)); } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java index afb1ff34d89c..5b7725e3bd2b 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Callback.java @@ -211,18 +211,6 @@ public void completed() completed.run(); } - @Override - public void succeeded() - { - completed(); - } - - @Override - public void failed(Throwable x) - { - completed(); - } - @Override public InvocationType getInvocationType() { @@ -334,28 +322,7 @@ public void failed(Throwable x) */ static Callback from(Callback callback1, Callback callback2) { - return new Callback() - { - @Override - public void succeeded() - { - callback1.succeeded(); - callback2.succeeded(); - } - - @Override - public void failed(Throwable x) - { - callback1.failed(x); - callback2.failed(x); - } - - @Override - public InvocationType getInvocationType() - { - return Invocable.combine(Invocable.getInvocationType(callback1), Invocable.getInvocationType(callback2)); - } - }; + return combine(callback1, callback2); } /** @@ -364,6 +331,18 @@ public InvocationType getInvocationType() interface Completing extends Callback { void completed(); + + @Override + default void succeeded() + { + completed(); + } + + @Override + default void failed(Throwable x) + { + completed(); + } } /** @@ -435,7 +414,45 @@ static Callback combine(Callback cb1, Callback cb2) if (cb2 == null) return cb1; - return from(cb1, cb2); + return new Callback() + { + @Override + public void succeeded() + { + try + { + cb1.succeeded(); + } + finally + { + cb2.succeeded(); + } + } + + @Override + public void failed(Throwable x) + { + try + { + cb1.failed(x); + } + catch (Throwable t) + { + if (ExceptionUtil.areNotAssociated(x, t)) + x.addSuppressed(t); + } + finally + { + cb2.failed(x); + } + } + + @Override + public InvocationType getInvocationType() + { + return Invocable.combine(Invocable.getInvocationType(cb1), Invocable.getInvocationType(cb2)); + } + }; } /**