From 8e57505ad9fd020f2f0241c06e5b57f20da248d2 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Tue, 11 May 2021 21:44:26 +0200 Subject: Janitor threadpool must be available across JettyHttpServer instances --- .../jdisc/http/server/jetty/JDiscContext.java | 6 +-- .../com/yahoo/jdisc/http/server/jetty/Janitor.java | 46 ++++++++++++++++++++++ .../jdisc/http/server/jetty/JettyHttpServer.java | 19 +-------- .../server/jetty/ServletOutputStreamWriter.java | 12 +++--- .../http/server/jetty/ServletRequestReader.java | 11 +++--- .../server/jetty/ServletResponseController.java | 5 +-- 6 files changed, 61 insertions(+), 38 deletions(-) create mode 100644 container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/Janitor.java (limited to 'container-core/src') diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java index b37a7352dc6..48c70095918 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscContext.java @@ -5,18 +5,16 @@ import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.http.ServerConfig; import com.yahoo.jdisc.service.CurrentContainer; -import java.util.concurrent.Executor; - public class JDiscContext { final FilterResolver filterResolver; final CurrentContainer container; - final Executor janitor; + final Janitor janitor; final Metric metric; final ServerConfig serverConfig; public JDiscContext(FilterBindings filterBindings, CurrentContainer container, - Executor janitor, + Janitor janitor, Metric metric, ServerConfig serverConfig) { diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/Janitor.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/Janitor.java new file mode 100644 index 00000000000..cd2b9ca23c0 --- /dev/null +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/Janitor.java @@ -0,0 +1,46 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.server.jetty; + +import com.google.inject.Inject; +import com.yahoo.component.AbstractComponent; +import com.yahoo.concurrent.DaemonThreadFactory; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.logging.Logger; + +/** + * Separate janitor threadpool for tasks that cannot be executed on the jdisc default threadpool due to risk of deadlock. + * Modelled as a separate component as the underlying executor must be available across {@link JettyHttpServer} instances. + * + * @author bjorncs + */ +public class Janitor extends AbstractComponent { + + private static final Logger log = Logger.getLogger(Janitor.class.getName()); + + private final ExecutorService executor; + + @Inject + public Janitor() { + int threadPoolSize = Math.max(2, Runtime.getRuntime().availableProcessors()/4); + log.info("Creating janitor executor with " + threadPoolSize + " threads"); + this.executor = Executors.newFixedThreadPool(threadPoolSize, new DaemonThreadFactory("jdisc-janitor-")); + } + + public void scheduleTask(Runnable task) { executor.execute(task); } + + @Override + public void deconstruct() { + try { + executor.shutdown(); + if (!executor.awaitTermination(10, TimeUnit.SECONDS)) { + log.warning("Failed to shutdown janitor in time"); + } + } catch (InterruptedException e) { + log.warning("Interrupted while shutting down janitor"); + Thread.currentThread().interrupt(); + } + } +} diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java index 510c561c10f..cf9945cc65b 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java @@ -4,8 +4,6 @@ package com.yahoo.jdisc.http.server.jetty; import com.google.inject.Inject; import com.yahoo.component.ComponentId; import com.yahoo.component.provider.ComponentRegistry; -import com.yahoo.concurrent.DaemonThreadFactory; -import com.yahoo.container.logging.AccessLog; import com.yahoo.container.logging.ConnectionLog; import com.yahoo.container.logging.RequestLog; import com.yahoo.jdisc.Metric; @@ -43,8 +41,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; import java.util.List; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -59,8 +55,6 @@ public class JettyHttpServer extends AbstractServerProvider { private final static Logger log = Logger.getLogger(JettyHttpServer.class.getName()); - private final ExecutorService janitor; - private final Server server; private final List listenedPorts = new ArrayList<>(); private final ServerMetricReporter metricsReporter; @@ -71,6 +65,7 @@ public class JettyHttpServer extends AbstractServerProvider { ServerConfig serverConfig, ServletPathsConfig servletPathsConfig, FilterBindings filterBindings, + Janitor janitor, ComponentRegistry connectorFactories, ComponentRegistry servletHolders, FilterInvoker filterInvoker, @@ -95,8 +90,6 @@ public class JettyHttpServer extends AbstractServerProvider { listenedPorts.add(connectorConfig.listenPort()); } - janitor = newJanitor(); - JDiscContext jDiscContext = new JDiscContext(filterBindings, container, janitor, @@ -208,15 +201,6 @@ public class JettyHttpServer extends AbstractServerProvider { return ports.stream().map(Object::toString).collect(Collectors.joining(":")); } - // Separate threadpool for tasks that cannot be executed on the jdisc default threadpool due to risk of deadlock - private static ExecutorService newJanitor() { - int threadPoolSize = Math.max(1, Runtime.getRuntime().availableProcessors()/8); - log.info("Creating janitor executor with " + threadPoolSize + " threads"); - return Executors.newFixedThreadPool( - threadPoolSize, - new DaemonThreadFactory(JettyHttpServer.class.getName() + "-Janitor-")); - } - @Override public void start() { try { @@ -258,7 +242,6 @@ public class JettyHttpServer extends AbstractServerProvider { } metricsReporter.shutdown(); - janitor.shutdown(); } private boolean isGracefulShutdownEnabled() { diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java index b4d03385c3b..696fd2d51ad 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java @@ -12,7 +12,6 @@ import java.util.ArrayList; import java.util.Deque; import java.util.Optional; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.Executor; import java.util.function.Consumer; import java.util.logging.Level; import java.util.logging.Logger; @@ -54,7 +53,7 @@ public class ServletOutputStreamWriter { // GuardedBy("state") private final ServletOutputStream outputStream; - private final Executor executor; + private final Janitor janitor; // GuardedBy("monitor") private final Deque responseContentQueue = new ArrayDeque<>(); @@ -70,9 +69,9 @@ public class ServletOutputStreamWriter { final CompletableFuture finishedFuture = new CompletableFuture<>(); - public ServletOutputStreamWriter(ServletOutputStream outputStream, Executor executor, RequestMetricReporter metricReporter) { + public ServletOutputStreamWriter(ServletOutputStream outputStream, Janitor janitor, RequestMetricReporter metricReporter) { this.outputStream = outputStream; - this.executor = executor; + this.janitor = janitor; this.metricReporter = metricReporter; } @@ -96,7 +95,7 @@ public class ServletOutputStreamWriter { synchronized (monitor) { if (state == State.FINISHED_OR_ERROR) { - executor.execute(() -> handler.failed(new IllegalStateException("ContentChannel already closed."))); + janitor.scheduleTask(() -> handler.failed(new IllegalStateException("ContentChannel already closed."))); return; } responseContentQueue.addLast(new ResponseContentPart(buf, handler)); @@ -207,8 +206,7 @@ public class ServletOutputStreamWriter { runCompletionHandler_logOnExceptions( () -> responseContentPart.handler.failed(failReason)); - executor.execute( - () -> failedParts.forEach(failCompletionHandler)); + janitor.scheduleTask(() -> failedParts.forEach(failCompletionHandler)); } private void writeBufferToOutputStream(ResponseContentPart contentPart) throws Throwable { diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java index 1882448757a..26d74bdccb3 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java @@ -10,7 +10,6 @@ import javax.servlet.ServletInputStream; import java.io.IOException; import java.nio.ByteBuffer; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.Executor; import java.util.logging.Level; import java.util.logging.Logger; @@ -42,7 +41,7 @@ class ServletRequestReader implements ReadListener { private final ServletInputStream servletInputStream; private final ContentChannel requestContentChannel; - private final Executor executor; + private final Janitor janitor; private final RequestMetricReporter metricReporter; private int bytesRead; @@ -93,17 +92,17 @@ class ServletRequestReader implements ReadListener { public ServletRequestReader( ServletInputStream servletInputStream, ContentChannel requestContentChannel, - Executor executor, + Janitor janitor, RequestMetricReporter metricReporter) { Preconditions.checkNotNull(servletInputStream); Preconditions.checkNotNull(requestContentChannel); - Preconditions.checkNotNull(executor); + Preconditions.checkNotNull(janitor); Preconditions.checkNotNull(metricReporter); this.servletInputStream = servletInputStream; this.requestContentChannel = requestContentChannel; - this.executor = executor; + this.janitor = janitor; this.metricReporter = metricReporter; } @@ -163,7 +162,7 @@ class ServletRequestReader implements ReadListener { } if (shouldCloseRequestContentChannel) { - executor.execute(this::closeCompletionHandler_noThrow); + janitor.scheduleTask(this::closeCompletionHandler_noThrow); } } diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java index 60b7878156f..31fa9e9ebaa 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java @@ -20,7 +20,6 @@ import java.nio.ByteBuffer; import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.Executor; import java.util.logging.Level; import java.util.logging.Logger; @@ -57,7 +56,7 @@ public class ServletResponseController { public ServletResponseController( HttpServletRequest servletRequest, HttpServletResponse servletResponse, - Executor executor, + Janitor janitor, RequestMetricReporter metricReporter, boolean developerMode) throws IOException { @@ -65,7 +64,7 @@ public class ServletResponseController { this.servletResponse = servletResponse; this.developerMode = developerMode; this.servletOutputStreamWriter = - new ServletOutputStreamWriter(servletResponse.getOutputStream(), executor, metricReporter); + new ServletOutputStreamWriter(servletResponse.getOutputStream(), janitor, metricReporter); } -- cgit v1.2.3