From b8887fb3fc82d9a86b360e3382c460ff10a65f23 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 8 Oct 2024 16:22:35 +0200 Subject: [PATCH 01/27] #12214 restore ee9 and ee10 thread starvation tests Signed-off-by: Ludovic Orban --- .../ee10/servlets/ThreadStarvationTest.java | 306 +++++++-------- .../ee9/servlets/ThreadStarvationTest.java | 350 +++++++++++------- 2 files changed, 376 insertions(+), 280 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index 4715a470fe20..0971d5a274a0 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -13,35 +13,44 @@ package org.eclipse.jetty.ee10.servlets; -import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.Socket; +import java.nio.ByteBuffer; import java.nio.channels.SelectionKey; import java.nio.channels.SocketChannel; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import java.util.concurrent.Exchanger; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; +import jakarta.servlet.ServletException; import org.eclipse.jetty.ee10.servlet.DefaultServlet; import org.eclipse.jetty.ee10.servlet.ServletContextHandler; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.io.ManagedSelector; import org.eclipse.jetty.io.SocketChannelEndPoint; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -59,7 +68,6 @@ public void dispose() throws Exception } @Test - @Tag("flaky") public void testDefaultServletSuccess() throws Exception { int maxThreads = 6; @@ -68,10 +76,10 @@ public void testDefaultServletSuccess() throws Exception _server = new Server(threadPool); // Prepare a big file to download. - File directory = MavenTestingUtils.getTargetTestingDir(); - Files.createDirectories(directory.toPath()); + Path directory = MavenTestingUtils.getTargetTestingPath(); + Files.createDirectories(directory); String resourceName = "resource.bin"; - Path resourcePath = Paths.get(directory.getPath(), resourceName); + Path resourcePath = directory.resolve(resourceName); try (OutputStream output = Files.newOutputStream(resourcePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) { byte[] chunk = new byte[256 * 1024]; @@ -105,8 +113,8 @@ protected void onIncompleteFlush() _server.addConnector(connector); ServletContextHandler context = new ServletContextHandler("/"); - context.setBaseResourceAsPath(directory.toPath()); - + context.setBaseResourceAsPath(directory); + //TODO: Uses DefaultServlet, currently all commented out context.addServlet(DefaultServlet.class, "/*").setAsyncSupported(false); _server.setHandler(context); @@ -223,172 +231,168 @@ public void run() } } - //TODO needs visibility of server.internal.HttpChannelState - /* @Test + @Test public void testFailureStarvation() throws Exception { - try (StacklessLogging stackless = new StacklessLogging(HttpChannelState.class)) + int acceptors = 0; + int selectors = 1; + int maxThreads = 10; + final int barried = maxThreads - acceptors - selectors * 2; + final CyclicBarrier barrier = new CyclicBarrier(barried); + + QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); + threadPool.setDetailedDump(true); + _server = new Server(threadPool); + + ServerConnector connector = new ServerConnector(_server, acceptors, selectors) { - int acceptors = 0; - int selectors = 1; - int maxThreads = 10; - final int barried = maxThreads - acceptors - selectors * 2; - final CyclicBarrier barrier = new CyclicBarrier(barried); - - QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); - threadPool.setDetailedDump(true); - _server = new Server(threadPool); - - ServerConnector connector = new ServerConnector(_server, acceptors, selectors) + @Override + protected SocketChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key) { - @Override - protected SocketChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key) + return new SocketChannelEndPoint(channel, selectSet, key, getScheduler()) { - return new SocketChannelEndPoint(channel, selectSet, key, getScheduler()) + @Override + public boolean flush(ByteBuffer... buffers) throws IOException { - @Override - public boolean flush(ByteBuffer... buffers) throws IOException - { - super.flush(buffers[0]); - throw new IOException("TEST FAILURE"); - } - }; - } - }; - connector.setIdleTimeout(Long.MAX_VALUE); - _server.addConnector(connector); - - final AtomicInteger count = new AtomicInteger(0); - class TheHandler extends Handler.Abstract + super.flush(buffers[0]); + throw new IOException("TEST FAILURE"); + } + }; + } + }; + connector.setIdleTimeout(Long.MAX_VALUE); + _server.addConnector(connector); + + final AtomicInteger count = new AtomicInteger(0); + class TheHandler extends Handler.Abstract + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception { - @Override - public boolean handle(request request, Response response, Callback callback) throws Exception + int c = count.getAndIncrement(); + try { - int c = count.getAndIncrement(); - try - { - if (c < barried) - { - barrier.await(10, TimeUnit.SECONDS); - } - } - catch (InterruptedException | BrokenBarrierException | TimeoutException e) + if (c < barried) { - throw new ServletException(e); + barrier.await(10, TimeUnit.SECONDS); } - - response.setStatus(200); - response.setContentLength(13); - response.write(true, callback, "Hello World!\n"); - return true; } + catch (InterruptedException | BrokenBarrierException | TimeoutException e) + { + throw new ServletException(e); + } + + response.setStatus(200); + response.getHeaders().put(HttpHeader.CONTENT_LENGTH, 13L); + response.write(true, BufferUtil.toBuffer("Hello World!\n"), callback); + return true; } - - _server.setHandler(new TheHandler()); - - _server.start(); - - List sockets = new ArrayList<>(); - for (int i = 0; i < maxThreads * 2; ++i) - { - Socket socket = new Socket("localhost", connector.getLocalPort()); - sockets.add(socket); - OutputStream output = socket.getOutputStream(); - String request = - "GET / HTTP/1.1\r\n" + - "Host: localhost\r\n" + - // "Connection: close\r\n" + - "\r\n"; - output.write(request.getBytes(StandardCharsets.UTF_8)); - output.flush(); - } - - byte[] buffer = new byte[48 * 1024]; - List> totals = new ArrayList<>(); - for (Socket socket : sockets) + } + + _server.setHandler(new TheHandler()); + + _server.start(); + + List sockets = new ArrayList<>(); + for (int i = 0; i < maxThreads * 2; ++i) + { + Socket socket = new Socket("localhost", connector.getLocalPort()); + sockets.add(socket); + OutputStream output = socket.getOutputStream(); + String request = + "GET / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + // "Connection: close\r\n" + + "\r\n"; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + } + + byte[] buffer = new byte[48 * 1024]; + List> totals = new ArrayList<>(); + for (Socket socket : sockets) + { + final Exchanger x = new Exchanger<>(); + totals.add(x); + final InputStream input = socket.getInputStream(); + + new Thread() { - final Exchanger x = new Exchanger<>(); - totals.add(x); - final InputStream input = socket.getInputStream(); - - new Thread() + @Override + public void run() { - @Override - public void run() + int read = 0; + try { - int read = 0; - try + // look for CRLFCRLF + StringBuilder header = new StringBuilder(); + int state = 0; + while (state < 4 && header.length() < 2048) { - // look for CRLFCRLF - StringBuilder header = new StringBuilder(); - int state = 0; - while (state < 4 && header.length() < 2048) + int ch = input.read(); + if (ch < 0) + break; + header.append((char)ch); + switch (state) { - int ch = input.read(); - if (ch < 0) + case 0: + if (ch == '\r') + state = 1; + break; + case 1: + if (ch == '\n') + state = 2; + else + state = 0; + break; + case 2: + if (ch == '\r') + state = 3; + else + state = 0; + break; + case 3: + if (ch == '\n') + state = 4; + else + state = 0; break; - header.append((char)ch); - switch (state) - { - case 0: - if (ch == '\r') - state = 1; - break; - case 1: - if (ch == '\n') - state = 2; - else - state = 0; - break; - case 2: - if (ch == '\r') - state = 3; - else - state = 0; - break; - case 3: - if (ch == '\n') - state = 4; - else - state = 0; - break; - } } - - read = input.read(buffer); } - catch (IOException e) + + read = input.read(buffer); + } + catch (IOException e) + { + e.printStackTrace(); + } + finally + { + try { - // e.printStackTrace(); + x.exchange(read); } - finally + catch (InterruptedException e) { - try - { - x.exchange(read); - } - catch (InterruptedException e) - { - e.printStackTrace(); - } + e.printStackTrace(); } } - }.start(); - } - - for (Exchanger x : totals) - { - Integer read = x.exchange(-1, 10, TimeUnit.SECONDS); - assertEquals(-1, read.intValue()); - } - - // We could read everything, good. - for (Socket socket : sockets) - { - socket.close(); - } - - _server.stop(); + } + }.start(); + } + + for (Exchanger x : totals) + { + Integer read = x.exchange(-1, 10, TimeUnit.SECONDS); + assertEquals(-1, read.intValue()); } - }*/ + + // We could read everything, good. + for (Socket socket : sockets) + { + socket.close(); + } + + _server.stop(); + } } diff --git a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java index dd5a3524a8c9..2b64a9ce7198 100644 --- a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java +++ b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.ee9.servlets; -import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -24,43 +23,37 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; +import java.util.concurrent.Exchanger; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import jakarta.servlet.ServletException; import org.eclipse.jetty.ee9.servlet.DefaultServlet; import org.eclipse.jetty.ee9.servlet.ServletContextHandler; import org.eclipse.jetty.http.HttpHeader; -import org.eclipse.jetty.http.HttpTester; -import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.ManagedSelector; import org.eclipse.jetty.io.SocketChannelEndPoint; -import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; public class ThreadStarvationTest @@ -75,7 +68,6 @@ public void dispose() throws Exception } @Test - @Tag("flaky") public void testDefaultServletSuccess() throws Exception { int maxThreads = 6; @@ -84,10 +76,10 @@ public void testDefaultServletSuccess() throws Exception _server = new Server(threadPool); // Prepare a big file to download. - File directory = MavenTestingUtils.getTargetTestingDir(); - Files.createDirectories(directory.toPath()); + Path directory = MavenTestingUtils.getTargetTestingPath(); + Files.createDirectories(directory); String resourceName = "resource.bin"; - Path resourcePath = Paths.get(directory.getPath(), resourceName); + Path resourcePath = directory.resolve(resourceName); try (OutputStream output = Files.newOutputStream(resourcePath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) { byte[] chunk = new byte[256 * 1024]; @@ -100,7 +92,7 @@ public void testDefaultServletSuccess() throws Exception } } - CountDownLatch writePending = new CountDownLatch(1); + final CountDownLatch writePending = new CountDownLatch(1); ServerConnector connector = new ServerConnector(_server, 0, 1) { @Override @@ -121,7 +113,7 @@ protected void onIncompleteFlush() _server.addConnector(connector); ServletContextHandler context = new ServletContextHandler(_server, "/"); - context.setResourceBase(directory.toURI().toString()); + context.setResourceBase(directory.toUri().toString()); context.addServlet(DefaultServlet.class, "/*").setAsyncSupported(false); _server.setHandler(context); @@ -144,37 +136,90 @@ protected void onIncompleteFlush() // Wait for a thread on the servlet to block. assertTrue(writePending.await(5, TimeUnit.SECONDS)); - ExecutorService executor = Executors.newCachedThreadPool(); - long expected = Files.size(resourcePath); - List> totals = new ArrayList<>(); + byte[] buffer = new byte[48 * 1024]; + List> totals = new ArrayList<>(); for (Socket socket : sockets) { - InputStream input = socket.getInputStream(); - totals.add(CompletableFuture.supplyAsync(() -> + final Exchanger x = new Exchanger<>(); + totals.add(x); + final InputStream input = socket.getInputStream(); + + new Thread() { - try - { - HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(input)); - if (response != null) - return response.getContentBytes().length; - return 0; - } - catch (IOException x) + @Override + public void run() { - x.printStackTrace(); - return -1; + long total = 0; + try + { + // look for CRLFCRLF + StringBuilder header = new StringBuilder(); + int state = 0; + while (state < 4 && header.length() < 2048) + { + int ch = input.read(); + if (ch < 0) + break; + header.append((char)ch); + switch (state) + { + case 0: + if (ch == '\r') + state = 1; + break; + case 1: + if (ch == '\n') + state = 2; + else + state = 0; + break; + case 2: + if (ch == '\r') + state = 3; + else + state = 0; + break; + case 3: + if (ch == '\n') + state = 4; + else + state = 0; + break; + } + } + + while (total < expected) + { + int read = input.read(buffer); + if (read < 0) + break; + total += read; + } + } + catch (IOException e) + { + e.printStackTrace(); + } + finally + { + try + { + x.exchange(total); + } + catch (InterruptedException e) + { + e.printStackTrace(); + } + } } - }, executor)); + }.start(); } - // Wait for all responses to arrive. - CompletableFuture.allOf(totals.toArray(new CompletableFuture[0])).get(20, TimeUnit.SECONDS); - - for (CompletableFuture total : totals) + for (Exchanger x : totals) { - assertFalse(total.isCompletedExceptionally()); - assertEquals(expected, total.get().intValue()); + Long total = x.exchange(-1L, 10000, TimeUnit.SECONDS); + assertEquals(expected, total.longValue()); } // We could read everything, good. @@ -182,123 +227,170 @@ protected void onIncompleteFlush() { socket.close(); } - - executor.shutdown(); - - _server.stop(); } @Test - @Tag("flaky") public void testFailureStarvation() throws Exception { - Logger serverInternalLogger = LoggerFactory.getLogger("org.eclipse.jetty.server.internal"); - try (StacklessLogging ignored = new StacklessLogging(serverInternalLogger)) - { - int acceptors = 0; - int selectors = 1; - int maxThreads = 10; - int parties = maxThreads - acceptors - selectors * 2; - CyclicBarrier barrier = new CyclicBarrier(parties); + int acceptors = 0; + int selectors = 1; + int maxThreads = 10; + final int barried = maxThreads - acceptors - selectors * 2; + final CyclicBarrier barrier = new CyclicBarrier(barried); - QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); - threadPool.setDetailedDump(true); - _server = new Server(threadPool); + QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); + threadPool.setDetailedDump(true); + _server = new Server(threadPool); - ServerConnector connector = new ServerConnector(_server, acceptors, selectors) + ServerConnector connector = new ServerConnector(_server, acceptors, selectors) + { + @Override + protected SocketChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key) { - @Override - protected SocketChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key) + return new SocketChannelEndPoint(channel, selectSet, key, getScheduler()) { - return new SocketChannelEndPoint(channel, selectSet, key, getScheduler()) + @Override + public boolean flush(ByteBuffer... buffers) throws IOException { - @Override - public boolean flush(ByteBuffer... buffers) throws IOException - { - // Write only the headers, then throw. - super.flush(buffers[0]); - throw new IOException("thrown by test"); - } - }; - } - }; - connector.setIdleTimeout(Long.MAX_VALUE); - _server.addConnector(connector); + super.flush(buffers[0]); + throw new IOException("TEST FAILURE"); + } + }; + } + }; + connector.setIdleTimeout(Long.MAX_VALUE); + _server.addConnector(connector); - AtomicInteger count = new AtomicInteger(0); - _server.setHandler(new Handler.Abstract() + final AtomicInteger count = new AtomicInteger(0); + class TheHandler extends Handler.Abstract + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception { - @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception + int c = count.getAndIncrement(); + try { - int c = count.getAndIncrement(); - if (c < parties) + if (c < barried) + { barrier.await(10, TimeUnit.SECONDS); - response.setStatus(200); - response.getHeaders().put(HttpHeader.CONTENT_LENGTH, 13); - Content.Sink.write(response, true, "Hello World!\n", callback); - return true; + } + } + catch (InterruptedException | BrokenBarrierException | TimeoutException e) + { + throw new ServletException(e); } - }); - - _server.start(); - List sockets = new ArrayList<>(); - for (int i = 0; i < maxThreads * 2; ++i) - { - Socket socket = new Socket("localhost", connector.getLocalPort()); - sockets.add(socket); - OutputStream output = socket.getOutputStream(); - String request = """ - GET / HTTP/1.1\r - Host: localhost\r - \r - """; - output.write(request.getBytes(StandardCharsets.UTF_8)); - output.flush(); + response.setStatus(200); + response.getHeaders().put(HttpHeader.CONTENT_LENGTH, 13L); + response.write(true, BufferUtil.toBuffer("Hello World!\n"), callback); + return true; } + } + + _server.setHandler(new TheHandler()); + + _server.start(); + + List sockets = new ArrayList<>(); + for (int i = 0; i < maxThreads * 2; ++i) + { + Socket socket = new Socket("localhost", connector.getLocalPort()); + sockets.add(socket); + OutputStream output = socket.getOutputStream(); + String request = + "GET / HTTP/1.1\r\n" + + "Host: localhost\r\n" + + // "Connection: close\r\n" + + "\r\n"; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + } - ExecutorService executor = Executors.newCachedThreadPool(); + byte[] buffer = new byte[48 * 1024]; + List> totals = new ArrayList<>(); + for (Socket socket : sockets) + { + final Exchanger x = new Exchanger<>(); + totals.add(x); + final InputStream input = socket.getInputStream(); - List> totals = new ArrayList<>(); - for (Socket socket : sockets) + new Thread() { - InputStream input = socket.getInputStream(); - totals.add(CompletableFuture.supplyAsync(() -> + @Override + public void run() { + int read = 0; try { - HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(input)); - if (response != null) - return response.getContentBytes().length; - return input.read(); + // look for CRLFCRLF + StringBuilder header = new StringBuilder(); + int state = 0; + while (state < 4 && header.length() < 2048) + { + int ch = input.read(); + if (ch < 0) + break; + header.append((char)ch); + switch (state) + { + case 0: + if (ch == '\r') + state = 1; + break; + case 1: + if (ch == '\n') + state = 2; + else + state = 0; + break; + case 2: + if (ch == '\r') + state = 3; + else + state = 0; + break; + case 3: + if (ch == '\n') + state = 4; + else + state = 0; + break; + } + } + + read = input.read(buffer); } - catch (Exception x) + catch (IOException e) { - x.printStackTrace(); - return -1; + e.printStackTrace(); } - }, executor)); - } - - // Wait for all responses to arrive. - CompletableFuture.allOf(totals.toArray(new CompletableFuture[0])).get(20, TimeUnit.SECONDS); - - for (CompletableFuture total : totals) - { - assertFalse(total.isCompletedExceptionally()); - assertEquals(-1, total.get().intValue()); - } - - // We could read everything, good. - for (Socket socket : sockets) - { - socket.close(); - } + finally + { + try + { + x.exchange(read); + } + catch (InterruptedException e) + { + e.printStackTrace(); + } + } + } + }.start(); + } - executor.shutdown(); + for (Exchanger x : totals) + { + Integer read = x.exchange(-1, 10, TimeUnit.SECONDS); + assertEquals(-1, read.intValue()); + } - _server.stop(); + // We could read everything, good. + for (Socket socket : sockets) + { + socket.close(); } + + _server.stop(); } } From 21fbc353ab1d667c19b2c467671a6894acefc6aa Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 16 Oct 2024 10:44:09 +0200 Subject: [PATCH 02/27] #12214 implement todos Signed-off-by: Ludovic Orban --- .../jetty/server/ThreadStarvationTest.java | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java index db42bfea6a4f..2a328db3711d 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java @@ -17,6 +17,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.Socket; +import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.ArrayList; @@ -32,6 +33,7 @@ import javax.net.ssl.SSLContext; import org.eclipse.jetty.io.ArrayByteBufferPool; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IO; @@ -39,7 +41,6 @@ import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -50,7 +51,6 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; -@Disabled // TODO public class ThreadStarvationTest { static final int BUFFER_SIZE = 1024 * 1024; @@ -257,18 +257,8 @@ protected static class ReadHandler extends Handler.Abstract public boolean handle(Request request, Response response, Callback callback) throws Exception { response.setStatus(200); - /* TODO - - int l = request.getContentLength(); - int r = 0; - while (r < l) - { - if (request.getInputStream().read() >= 0) - r++; - } - - response.write(true, callback, ByteBuffer.wrap(("Read Input " + r + "\r\n").getBytes())); - */ + String string = Content.Source.asString(request); + response.write(true, ByteBuffer.wrap(("Read Input " + string.length() + "\r\n").getBytes()), callback); return true; } } @@ -361,19 +351,17 @@ protected static class WriteHandler extends Handler.Abstract @Override public boolean handle(Request request, Response response, Callback callback) throws Exception { - /* TODO - baseRequest.setHandled(true); response.setStatus(200); - response.setContentLength(BUFFERS * BUFFER_SIZE); - OutputStream out = response.getOutputStream(); - for (int i = 0; i < BUFFERS; i++) + try (OutputStream out = Content.Sink.asOutputStream(response)) { - out.write(content); - out.flush(); + for (int i = 0; i < BUFFERS; i++) + { + out.write(content); + out.flush(); + } } - */ return true; } } From 4b557e41d5dac34d1d36ffe32441ee5d665a80d0 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 16 Oct 2024 15:22:30 +0200 Subject: [PATCH 03/27] #12214 add read starvation test to ee* envs Signed-off-by: Ludovic Orban --- .../ee10/servlets/ThreadStarvationTest.java | 90 +++++++++++++++++++ .../ee9/servlets/ThreadStarvationTest.java | 90 +++++++++++++++++++ 2 files changed, 180 insertions(+) diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index 0971d5a274a0..3c5ddc6bcfcd 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -28,16 +28,24 @@ import java.util.Arrays; import java.util.List; import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.Exchanger; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.eclipse.jetty.ee10.servlet.DefaultServlet; import org.eclipse.jetty.ee10.servlet.ServletContextHandler; +import org.eclipse.jetty.ee10.servlet.ServletHolder; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.io.ManagedSelector; import org.eclipse.jetty.io.SocketChannelEndPoint; @@ -49,10 +57,13 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -67,6 +78,85 @@ public void dispose() throws Exception _server.stop(); } + @Test + public void testReadStarvation() throws Exception + { + int maxThreads = 5; + int clients = maxThreads + 2; + QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); + threadPool.setDetailedDump(true); + _server = new Server(threadPool); + + ServerConnector connector = new ServerConnector(_server, 1, 1); + _server.addConnector(connector); + + ServletContextHandler context = new ServletContextHandler("/"); + context.addServlet(new ServletHolder(new HttpServlet() { + @Override + protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + IO.copy(req.getInputStream(), resp.getOutputStream()); + } + }), "/*"); + _server.setHandler(context); + + _server.start(); + + ExecutorService clientExecutors = Executors.newFixedThreadPool(clients); + + List> clientTasks = new ArrayList<>(); + + for (int i = 0; i < clients; i++) + { + clientTasks.add(() -> + { + try (Socket client = new Socket("localhost", connector.getLocalPort()); + OutputStream out = client.getOutputStream(); + InputStream in = client.getInputStream()) + { + client.setSoTimeout(10000); + + String request = + "PUT / HTTP/1.0\r\n" + + "host: localhost\r\n" + + "content-length: 10\r\n" + + "\r\n" + + "1"; + + // Write partial request + out.write(request.getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Finish Request + Thread.sleep(1500); + out.write(("234567890\r\n").getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Read Response + String response = IO.toString(in); + assertEquals(-1, in.read()); + return response; + } + }); + } + + try + { + List> responses = clientExecutors.invokeAll(clientTasks, 60, TimeUnit.SECONDS); + + for (Future responseFut : responses) + { + String response = responseFut.get(); + assertThat(response, containsString("200 OK")); + assertThat(response, containsString("Read Input 10")); + } + } + finally + { + clientExecutors.shutdownNow(); + } + } + @Test public void testDefaultServletSuccess() throws Exception { diff --git a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java index 2b64a9ce7198..da30254d2e99 100644 --- a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java +++ b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java @@ -28,16 +28,24 @@ import java.util.Arrays; import java.util.List; import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.Exchanger; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.eclipse.jetty.ee9.servlet.DefaultServlet; import org.eclipse.jetty.ee9.servlet.ServletContextHandler; +import org.eclipse.jetty.ee9.servlet.ServletHolder; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.io.ManagedSelector; import org.eclipse.jetty.io.SocketChannelEndPoint; @@ -49,10 +57,13 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -67,6 +78,85 @@ public void dispose() throws Exception _server.stop(); } + @Test + public void testReadStarvation() throws Exception + { + int maxThreads = 5; + int clients = maxThreads + 2; + QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); + threadPool.setDetailedDump(true); + _server = new Server(threadPool); + + ServerConnector connector = new ServerConnector(_server, 1, 1); + _server.addConnector(connector); + + ServletContextHandler context = new ServletContextHandler(_server, "/"); + context.addServlet(new ServletHolder(new HttpServlet() { + @Override + protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + IO.copy(req.getInputStream(), resp.getOutputStream()); + } + }), "/*"); + _server.setHandler(context); + + _server.start(); + + ExecutorService clientExecutors = Executors.newFixedThreadPool(clients); + + List> clientTasks = new ArrayList<>(); + + for (int i = 0; i < clients; i++) + { + clientTasks.add(() -> + { + try (Socket client = new Socket("localhost", connector.getLocalPort()); + OutputStream out = client.getOutputStream(); + InputStream in = client.getInputStream()) + { + client.setSoTimeout(10000); + + String request = + "PUT / HTTP/1.0\r\n" + + "host: localhost\r\n" + + "content-length: 10\r\n" + + "\r\n" + + "1"; + + // Write partial request + out.write(request.getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Finish Request + Thread.sleep(1500); + out.write(("234567890\r\n").getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Read Response + String response = IO.toString(in); + assertEquals(-1, in.read()); + return response; + } + }); + } + + try + { + List> responses = clientExecutors.invokeAll(clientTasks, 60, TimeUnit.SECONDS); + + for (Future responseFut : responses) + { + String response = responseFut.get(); + assertThat(response, containsString("200 OK")); + assertThat(response, containsString("Read Input 10")); + } + } + finally + { + clientExecutors.shutdownNow(); + } + } + @Test public void testDefaultServletSuccess() throws Exception { From e9dc2ebe3cbf7baafcf614d326847f36ea362434 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 17 Oct 2024 11:35:56 +1100 Subject: [PATCH 04/27] Fixed EE10 read starvation --- .../jetty/server/handler/ContextRequest.java | 8 ++++- .../ee10/servlet/AsyncContentProducer.java | 36 +++++++++++++------ .../eclipse/jetty/ee10/servlet/HttpInput.java | 25 ++++++++----- .../jetty/ee10/servlet/HttpOutput.java | 5 ++- .../jetty/ee10/servlet/ServletChannel.java | 4 +-- .../ee10/servlets/ThreadStarvationTest.java | 2 +- 6 files changed, 53 insertions(+), 27 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java index 1a4eb4b30196..01d437a5e0df 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java @@ -59,7 +59,7 @@ public Context getContext() return _context; } - private class OnContextDemand implements Runnable + private class OnContextDemand implements Runnable, Invocable { private final Runnable _demandCallback; @@ -73,5 +73,11 @@ public void run() { _context.run(_demandCallback, ContextRequest.this); } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(_demandCallback); + } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index 3aefcdf60e6d..a2c1a37ed651 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.util.NanoTime; import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,7 +31,7 @@ * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextChunk()} will never block * but will return null when there is no available content. */ -class AsyncContentProducer implements ContentProducer +class AsyncContentProducer implements ContentProducer, Invocable, Runnable { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final Content.Chunk RECYCLED_ERROR_CHUNK = Content.Chunk.from(new StaticException("ContentProducer has been recycled"), true); @@ -250,16 +251,7 @@ public boolean isReady() } state.onReadUnready(); - _servletChannel.getRequest().demand(() -> - { - if (LOG.isDebugEnabled()) - LOG.debug("isReady() demand callback {}", this); - // We could call this.onContentProducible() directly but this - // would mean we would need to take the lock here while it - // is the responsibility of the HttpInput to take it. - if (_servletChannel.getHttpInput().onContentProducible()) - _servletChannel.handle(); - }); + _servletChannel.getRequest().demand(this); if (LOG.isDebugEnabled()) LOG.debug("isReady(), no chunk {}", this); @@ -271,6 +263,28 @@ boolean isUnready() return _servletChannel.getServletRequestState().isInputUnready(); } + /** + * This run method is used as the Runnable for demand + */ + @Override + public void run() + { + if (LOG.isDebugEnabled()) + LOG.debug("isReady() demand callback {}", this); + // We could call this.onContentProducible() directly but this + // would mean we would need to take the lock here while it + // is the responsibility of the HttpInput to take it. + if (_servletChannel.getHttpInput().onContentProducible()) + _servletChannel.handle(); + } + + @Override + public InvocationType getInvocationType() + { + // This is the invocation type when the producer is passed as demand, so ask the HttpInput. + return _servletChannel.getHttpInput().getInvocationType(); + } + /** * Never returns an empty chunk that isn't a failure and/or last. */ diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index de0f83f8a0e9..2da49fc12de4 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.Context; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,7 +31,7 @@ *

While this class is-a Runnable, it should never be dispatched in it's own thread. It is a runnable only so that the calling thread can use {@link * Context#run(Runnable)} to setup classloaders etc.

*/ -public class HttpInput extends ServletInputStream implements Runnable +public class HttpInput extends ServletInputStream implements Invocable { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); @@ -159,6 +160,19 @@ public boolean isAsync() return _readListener != null; } + @Override + public Invocable.InvocationType getInvocationType() + { + // This is the invocation type used for demand callbacks. + // If we are blocking mode, then we implement the callbacks, which just wake up the blocked application thread + // If we are async mode, then we need to ask the read listener (which will probably be seen as blocking as + // the InvocationType API is normally hidden from a web application. + // TODO is there another way for an app to promise its callbacks are not blocking? + return _readListener == null + ? Invocable.InvocationType.NON_BLOCKING // blocking reads have non blocking callbacks + : Invocable.getInvocationType(_readListener); + } + /* ServletInputStream */ @Override @@ -316,14 +330,7 @@ public int available() } } - /* Runnable */ - - /* - *

While this class is-a Runnable, it should never be dispatched in it's own thread. It is a runnable only so that the calling thread can use {@link - * ContextHandler#handle(Runnable)} to setup classloaders etc.

- */ - @Override - public void run() + public void readCallback() { Content.Chunk chunk; try (AutoLock lock = _lock.lock()) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index 0ac43556411e..0a130ec1fe7f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -59,7 +59,7 @@ * via {@link RequestDispatcher#include(ServletRequest, ServletResponse)} to * close the stream, to be reopened after the inclusion ends.

*/ -public class HttpOutput extends ServletOutputStream implements Runnable +public class HttpOutput extends ServletOutputStream { /** * The output state @@ -1352,8 +1352,7 @@ public boolean isReady() } } - @Override - public void run() + public void writeCallback() { Throwable error = null; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 4a6ebb620d4a..cfa6f3ff2d5c 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -539,13 +539,13 @@ public void handle() case READ_CALLBACK: { - _context.run(() -> _servletContextRequest.getHttpInput().run()); + _context.run(_servletContextRequest.getHttpInput()::readCallback); break; } case WRITE_CALLBACK: { - _context.run(() -> _servletContextRequest.getHttpOutput().run()); + _context.run(() -> _servletContextRequest.getHttpOutput().writeCallback()); break; } diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index 3c5ddc6bcfcd..f1049b15594f 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -148,7 +148,7 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IO { String response = responseFut.get(); assertThat(response, containsString("200 OK")); - assertThat(response, containsString("Read Input 10")); + assertThat(response, containsString("1234567890")); } } finally From f95a3e41e18b3117d31def110f12a8cd64b5955a Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 17 Oct 2024 15:24:30 +0200 Subject: [PATCH 05/27] #12214 fix Content.Source.asString() so its unblocking task is NON_BLOCKING Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/io/Content.java | 6 +- .../io/internal/ContentSourceString.java | 3 +- .../java/org/eclipse/jetty/util/Blocker.java | 59 +++++++++++++++++++ .../eclipse/jetty/util/thread/Invocable.java | 2 + 4 files changed, 68 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index d622481f5ed4..7dbaf9144099 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -410,7 +410,11 @@ static String asString(Source source, Charset charset) throws IOException { try { - return asStringAsync(source, charset).get(); + try (Blocker.Promise promise = Blocker.promise()) + { + asString(source, charset, promise); + return promise.block(); + } } catch (Throwable x) { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java index 9ae9605dd254..8c795437a619 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java @@ -18,6 +18,7 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.CharsetStringBuilder; import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.thread.Invocable; public class ContentSourceString { @@ -39,7 +40,7 @@ public void convert() Content.Chunk chunk = content.read(); if (chunk == null) { - content.demand(this::convert); + content.demand(Invocable.from(Invocable.getInvocationType(promise), this::convert)); return; } if (Content.Chunk.isFailure(chunk)) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Blocker.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Blocker.java index 2aa2c1814b78..09bd401b52bb 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Blocker.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Blocker.java @@ -206,6 +206,65 @@ public void close() }; } + public interface Promise extends org.eclipse.jetty.util.Promise, AutoCloseable, Invocable + { + C block() throws IOException; + + @Override + void close(); + } + + public static Promise promise() + { + return new Promise<>() + { + private final CompletableFuture _future = new CompletableFuture<>(); + + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + + @Override + public C block() throws IOException + { + try + { + return _future.get(); + } + catch (Throwable t) + { + throw IO.rethrow(t); + } + } + + @Override + public void close() + { + if (!_future.isDone()) + { + if (LOG.isDebugEnabled()) + LOG.warn("Blocking.Promise incomplete", new Throwable()); + else + LOG.warn("Blocking.Promise incomplete"); + } + } + + @Override + public void succeeded(C result) + { + _future.complete(result); + } + + @Override + public void failed(Throwable x) + { + _future.completeExceptionally(x); + } + }; + } + /** * A shared reusable Blocking source. */ diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index 19586df42bf4..e6307b4ce4e6 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -127,6 +127,8 @@ public String toString() */ static Task from(InvocationType type, Runnable task) { + if (task instanceof Task t && t.getInvocationType() == type) + return t; return new ReadyTask(type, task); } From b61d6ad6a889ede0c6b55776b926faacafe68b45 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 17 Oct 2024 15:46:38 +0200 Subject: [PATCH 06/27] #12214 fix ee9 testReadStarvation Signed-off-by: Ludovic Orban --- .../eclipse/jetty/ee9/nested/HttpChannel.java | 6 ++++-- .../org/eclipse/jetty/ee9/nested/HttpInput.java | 16 +++++++++++++++- .../jetty/ee9/servlets/ThreadStarvationTest.java | 2 +- 3 files changed, 20 insertions(+), 4 deletions(-) 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 f705b229362d..3b1b367b987f 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 @@ -59,6 +59,7 @@ import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.SharedBlockingCallback.Blocker; +import org.eclipse.jetty.util.thread.Invocable; import org.eclipse.jetty.util.thread.Scheduler; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -155,11 +156,12 @@ public ConnectionMetaData getConnectionMetaData() public boolean needContent() { // TODO: optimize by attempting a read? - getCoreRequest().demand(() -> + Invocable.InvocationType invocationType = getRequest().getHttpInput().getInvocationType(); + getCoreRequest().demand(Invocable.from(invocationType, () -> { if (getRequest().getHttpInput().onContentProducible()) handle(); - }); + })); return false; } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java index 57782e9c297f..f0a5d1aa6953 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java @@ -26,6 +26,7 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.component.Destroyable; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -33,7 +34,7 @@ *

While this class is-a Runnable, it should never be dispatched in it's own thread. It is a runnable only so that the calling thread can use {@link * Context#run(Runnable)} to setup classloaders etc.

*/ -public class HttpInput extends ServletInputStream implements Runnable +public class HttpInput extends ServletInputStream implements Invocable.Task { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); @@ -365,6 +366,19 @@ public int available() } } + @Override + public Invocable.InvocationType getInvocationType() + { + // This is the invocation type used for demand callbacks. + // If we are blocking mode, then we implement the callbacks, which just wake up the blocked application thread + // If we are async mode, then we need to ask the read listener (which will probably be seen as blocking as + // the InvocationType API is normally hidden from a web application. + // TODO is there another way for an app to promise its callbacks are not blocking? + return _readListener == null + ? Invocable.InvocationType.NON_BLOCKING // blocking reads have non blocking callbacks + : Invocable.getInvocationType(_readListener); + } + /* Runnable */ /* diff --git a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java index da30254d2e99..26f6feddeaa7 100644 --- a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java +++ b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java @@ -148,7 +148,7 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IO { String response = responseFut.get(); assertThat(response, containsString("200 OK")); - assertThat(response, containsString("Read Input 10")); + assertThat(response, containsString("1234567890")); } } finally From 5df96ea13c4e0822fef28c1d3bdd30379c776796 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 17 Oct 2024 18:39:34 +0200 Subject: [PATCH 07/27] fix checkstyle Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java | 3 ++- .../org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index f1049b15594f..732e1be23dab 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -91,7 +91,8 @@ public void testReadStarvation() throws Exception _server.addConnector(connector); ServletContextHandler context = new ServletContextHandler("/"); - context.addServlet(new ServletHolder(new HttpServlet() { + context.addServlet(new ServletHolder(new HttpServlet() + { @Override protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException { diff --git a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java index 26f6feddeaa7..9e8ef884a58e 100644 --- a/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java +++ b/jetty-ee9/jetty-ee9-servlets/src/test/java/org/eclipse/jetty/ee9/servlets/ThreadStarvationTest.java @@ -91,7 +91,8 @@ public void testReadStarvation() throws Exception _server.addConnector(connector); ServletContextHandler context = new ServletContextHandler(_server, "/"); - context.addServlet(new ServletHolder(new HttpServlet() { + context.addServlet(new ServletHolder(new HttpServlet() + { @Override protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException { From 2ff0e6eacb4780bf0203e920a452d91836bf333c Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 18 Oct 2024 14:06:26 +1100 Subject: [PATCH 08/27] Use Invocable.InvocableCompletableFuture for ContentSourceCompletableFuture --- .../ContentSourceCompletableFuture.java | 18 ++- .../eclipse/jetty/util/thread/Invocable.java | 119 ++++++++++++++++++ .../ee10/servlets/ThreadStarvationTest.java | 94 ++++++++++++++ 3 files changed, 225 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java index 6461f9489c1a..81e38f41b1a4 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java @@ -14,13 +14,13 @@ package org.eclipse.jetty.io.content; import java.io.EOFException; -import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.util.thread.Invocable; /** *

A utility class to convert content from a {@link Content.Source} to an instance - * available via a {@link CompletableFuture}.

+ * available via a {@link java.util.concurrent.CompletableFuture}.

*

An example usage to asynchronously read UTF-8 content is:

* 
{@code
  * public static class CompletableUTF8String extends ContentSourceCompletableFuture;
@@ -53,7 +53,7 @@
  * String s = cs.get();
  * }
*/ -public abstract class ContentSourceCompletableFuture extends CompletableFuture +public abstract class ContentSourceCompletableFuture extends Invocable.InvocableCompletableFuture implements Runnable { private final Content.Source _content; @@ -66,12 +66,12 @@ public ContentSourceCompletableFuture(Content.Source content) *

Initiates the parsing of the {@link Content.Source}.

*

For every valid chunk that is read, {@link #parse(Content.Chunk)} * is called, until a result is produced that is used to - * complete this {@link CompletableFuture}.

+ * complete this {@link java.util.concurrent.CompletableFuture}.

*

Internally, this method is called multiple times to progress * the parsing in response to {@link Content.Source#demand(Runnable)} * calls.

*

Exceptions thrown during parsing result in this - * {@link CompletableFuture} to be completed exceptionally.

+ * {@link java.util.concurrent.CompletableFuture} to be completed exceptionally.

*/ public void parse() { @@ -80,7 +80,7 @@ public void parse() Content.Chunk chunk = _content.read(); if (chunk == null) { - _content.demand(this::parse); + _content.demand(this); return; } if (Content.Chunk.isFailure(chunk)) @@ -126,6 +126,12 @@ public void parse() } } + @Override + public void run() + { + parse(); + } + /** *

Called by {@link #parse()} to parse a {@link org.eclipse.jetty.io.Content.Chunk}.

* diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index e6307b4ce4e6..a2a90a67059f 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -13,6 +13,14 @@ package org.eclipse.jetty.util.thread; +import java.util.Objects; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiConsumer; +import java.util.function.BiFunction; +import java.util.function.Consumer; +import java.util.function.Function; + /** *

A task (typically either a {@link Runnable} or {@link Callable} * that declares how it will behave when invoked:

@@ -243,4 +251,115 @@ public InvocationType getInvocationType() } return result; } + + /** + * An extension of {@link java.util.concurrent.CompletableFuture} that is an {@link Invocable}. + * The {@link InvocationType} is initially the type used in construction (default NON_BLOCKING). + * If a non async method is called, then the invocation type of any passed function is used. + * @param + */ + class InvocableCompletableFuture extends java.util.concurrent.CompletableFuture implements Invocable + { + private final AtomicReference _invocationType = new AtomicReference<>(); + + public InvocableCompletableFuture() + { + this(null); + } + + public InvocableCompletableFuture(InvocationType invocationType) + { + _invocationType.set(Objects.requireNonNullElse(invocationType, InvocationType.NON_BLOCKING)); + } + + @Override + public InvocationType getInvocationType() + { + return _invocationType.get(); + } + + @Override + public java.util.concurrent.CompletableFuture acceptEither(CompletionStage other, Consumer action) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + return super.acceptEither(other, action); + } + + @Override + public java.util.concurrent.CompletableFuture applyToEither(CompletionStage other, Function fn) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(fn))); + return super.applyToEither(other, fn); + } + + @Override + public java.util.concurrent.CompletableFuture handle(BiFunction fn) + { + _invocationType.set(Invocable.getInvocationType(fn)); + return super.handle(fn); + } + + @Override + public java.util.concurrent.CompletableFuture runAfterBoth(CompletionStage other, Runnable action) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + return super.runAfterBoth(other, action); + } + + @Override + public java.util.concurrent.CompletableFuture runAfterEither(CompletionStage other, Runnable action) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + return super.runAfterEither(other, action); + } + + @Override + public java.util.concurrent.CompletableFuture thenAccept(Consumer action) + { + _invocationType.set(Invocable.getInvocationType(action)); + return super.thenAccept(action); + } + + @Override + public java.util.concurrent.CompletableFuture thenAcceptBoth(CompletionStage other, BiConsumer action) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + return super.thenAcceptBoth(other, action); + } + + @Override + public java.util.concurrent.CompletableFuture thenApply(Function fn) + { + _invocationType.set(Invocable.getInvocationType(fn)); + return super.thenApply(fn); + } + + @Override + public java.util.concurrent.CompletableFuture thenCombine(CompletionStage other, BiFunction fn) + { + _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(fn))); + return super.thenCombine(other, fn); + } + + @Override + public java.util.concurrent.CompletableFuture thenCompose(Function> fn) + { + _invocationType.set(Invocable.getInvocationType(fn)); + return super.thenCompose(fn); + } + + @Override + public java.util.concurrent.CompletableFuture thenRun(Runnable action) + { + _invocationType.set(Invocable.getInvocationType(action)); + return super.thenRun(action); + } + + @Override + public java.util.concurrent.CompletableFuture whenComplete(BiConsumer action) + { + _invocationType.set(Invocable.getInvocationType(action)); + return super.whenComplete(action); + } + } } diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index 732e1be23dab..965e93546935 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -158,6 +158,100 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IO } } + @Test + public void testFormStarvation() throws Exception + { + int maxThreads = 5; + int clients = maxThreads + 2; + QueuedThreadPool threadPool = new QueuedThreadPool(maxThreads, maxThreads); + threadPool.setDetailedDump(true); + _server = new Server(threadPool); + + ServerConnector connector = new ServerConnector(_server, 1, 1); + _server.addConnector(connector); + + ServletContextHandler context = new ServletContextHandler("/"); + context.addServlet(new ServletHolder(new HttpServlet() { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + resp.setStatus(200); + req.getParameterMap().forEach((key, value) -> + { + try + { + resp.getWriter().printf("%s=%s\n", key, Arrays.asList(value)); + } + catch (IOException ex) + { + throw new RuntimeException(ex); + } + }); + } + }), "/*"); + _server.setHandler(context); + + _server.start(); + + ExecutorService clientExecutors = Executors.newFixedThreadPool(clients); + + List> clientTasks = new ArrayList<>(); + + for (int i = 0; i < clients; i++) + { + clientTasks.add(() -> + { + try (Socket client = new Socket("localhost", connector.getLocalPort()); + OutputStream out = client.getOutputStream(); + InputStream in = client.getInputStream()) + { + client.setSoTimeout(10000); + + String request = """ + POST / HTTP/1.0\r + host: localhost\r + content-type: application/x-www-form-urlencoded\r + content-length: 11\r + \r + a=1&b"""; + + // Write partial request + out.write(request.getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Finish Request + Thread.sleep(1500); + out.write(("=2&c=3\r\n").getBytes(StandardCharsets.UTF_8)); + out.flush(); + + // Read Response + String response = IO.toString(in); + assertEquals(-1, in.read()); + return response; + } + }); + } + + try + { + List> responses = clientExecutors.invokeAll(clientTasks, 60, TimeUnit.SECONDS); + + for (Future responseFut : responses) + { + String response = responseFut.get(); + assertThat(response, containsString("200 OK")); + assertThat(response, containsString("a=[1]")); + assertThat(response, containsString("b=[2]")); + assertThat(response, containsString("c=[3]")); + } + } + finally + { + clientExecutors.shutdownNow(); + } + } + + @Test public void testDefaultServletSuccess() throws Exception { From 24c180a9b105ac2e1c4e274d237fcbd38d0bfe22 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 18 Oct 2024 16:07:15 +1100 Subject: [PATCH 09/27] more demand invocables --- .../jetty/io/content/ContentSourcePublisher.java | 14 ++++++++++++-- .../jetty/io/content/ContentSourceTransformer.java | 3 ++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java index 0e3642daadfb..161862f99cf7 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourcePublisher.java @@ -127,7 +127,7 @@ public void cancel() } } - private static final class ActiveSubscription extends IteratingCallback implements Flow.Subscription + private static final class ActiveSubscription extends IteratingCallback implements Flow.Subscription, Runnable { private static final long NO_MORE_DEMAND = -1; private static final Throwable COMPLETED = new StaticException("Source.Content read fully"); @@ -184,7 +184,8 @@ else if (!(cancelled instanceof SuppressedException)) if (chunk == null) { - content.demand(this::succeeded); + // Pass this, which is Invocable + content.demand(this); return Action.SCHEDULED; } @@ -219,6 +220,15 @@ else if (!(cancelled instanceof SuppressedException)) return Action.IDLE; } + /** + * Called by {@link Content.Source#demand(Runnable)} + */ + @Override + public void run() + { + succeeded(); + } + @Override public void request(long n) { diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java index c4a6a9fe9c54..66f75502e4d5 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java @@ -17,6 +17,7 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.ExceptionUtil; +import org.eclipse.jetty.util.thread.Invocable; import org.eclipse.jetty.util.thread.SerializedInvoker; /** @@ -99,7 +100,7 @@ public void demand(Runnable demandCallback) { this.demandCallback = Objects.requireNonNull(demandCallback); if (needsRawRead) - rawSource.demand(() -> invoker.run(this::invokeDemandCallback)); + rawSource.demand(Invocable.from(Invocable.getInvocationType(demandCallback), () -> invoker.run(this::invokeDemandCallback))); else invoker.run(this::invokeDemandCallback); } From 15a59caec2495923e55869b758e95a480d984800 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 10:38:59 +0200 Subject: [PATCH 10/27] fix checkstyle Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java index 965e93546935..d46d3644dea9 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java +++ b/jetty-ee10/jetty-ee10-servlets/src/test/java/org/eclipse/jetty/ee10/servlets/ThreadStarvationTest.java @@ -171,7 +171,8 @@ public void testFormStarvation() throws Exception _server.addConnector(connector); ServletContextHandler context = new ServletContextHandler("/"); - context.addServlet(new ServletHolder(new HttpServlet() { + context.addServlet(new ServletHolder(new HttpServlet() + { @Override protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { @@ -251,7 +252,6 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I } } - @Test public void testDefaultServletSuccess() throws Exception { From d77ac47fae4492c2b601987d0cfec3efa53d06f4 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 11:18:34 +0200 Subject: [PATCH 11/27] cleanup ee9 fix Signed-off-by: Ludovic Orban --- .../eclipse/jetty/ee9/nested/HttpChannel.java | 25 ++++++++++++++----- .../eclipse/jetty/ee9/nested/HttpInput.java | 5 ++-- 2 files changed, 21 insertions(+), 9 deletions(-) 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 3b1b367b987f..5e1f6cbeb6ed 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 @@ -87,6 +87,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor private final Listener _combinedListener; private final Dispatchable _requestDispatcher; private final Dispatchable _asyncDispatcher; + private final NeedContentInvocableTask _needContentTask; @Deprecated private final List _transientListeners = new ArrayList<>(); private MetaData.Response _committedMetaData; @@ -114,6 +115,7 @@ public HttpChannel(ContextHandler contextHandler, ConnectionMetaData connectionM _combinedListener = new HttpChannelListeners(_connector.getBeans(Listener.class)); _requestDispatcher = new RequestDispatchable(); _asyncDispatcher = new AsyncDispatchable(); + _needContentTask = new NeedContentInvocableTask(); if (LOG.isDebugEnabled()) LOG.debug("new {} -> {},{},{}", @@ -156,12 +158,7 @@ public ConnectionMetaData getConnectionMetaData() public boolean needContent() { // TODO: optimize by attempting a read? - Invocable.InvocationType invocationType = getRequest().getHttpInput().getInvocationType(); - getCoreRequest().demand(Invocable.from(invocationType, () -> - { - if (getRequest().getHttpInput().onContentProducible()) - handle(); - })); + getCoreRequest().demand(_needContentTask); return false; } @@ -1604,4 +1601,20 @@ public void dispatch() throws IOException, ServletException _request.setHandled(true); } } + + private class NeedContentInvocableTask implements Invocable.Task + { + @Override + public InvocationType getInvocationType() + { + return getRequest().getHttpInput().getReadListenerInvocationType(); + } + + @Override + public void run() + { + if (getRequest().getHttpInput().onContentProducible()) + handle(); + } + } } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java index f0a5d1aa6953..6c7ac24c64f3 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpInput.java @@ -34,7 +34,7 @@ *

While this class is-a Runnable, it should never be dispatched in it's own thread. It is a runnable only so that the calling thread can use {@link * Context#run(Runnable)} to setup classloaders etc.

*/ -public class HttpInput extends ServletInputStream implements Invocable.Task +public class HttpInput extends ServletInputStream implements Runnable { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); @@ -366,8 +366,7 @@ public int available() } } - @Override - public Invocable.InvocationType getInvocationType() + public Invocable.InvocationType getReadListenerInvocationType() { // This is the invocation type used for demand callbacks. // If we are blocking mode, then we implement the callbacks, which just wake up the blocked application thread From 0169d15e3903403bbaeb8274da38d50b72e79c6f Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 11:19:12 +0200 Subject: [PATCH 12/27] cleanup ee10 fix Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java | 4 ++-- .../main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java | 5 ++--- .../java/org/eclipse/jetty/ee10/servlet/ServletChannel.java | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index a2c1a37ed651..37897ad10faf 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -31,7 +31,7 @@ * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextChunk()} will never block * but will return null when there is no available content. */ -class AsyncContentProducer implements ContentProducer, Invocable, Runnable +class AsyncContentProducer implements ContentProducer, Invocable.Task { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final Content.Chunk RECYCLED_ERROR_CHUNK = Content.Chunk.from(new StaticException("ContentProducer has been recycled"), true); @@ -282,7 +282,7 @@ public void run() public InvocationType getInvocationType() { // This is the invocation type when the producer is passed as demand, so ask the HttpInput. - return _servletChannel.getHttpInput().getInvocationType(); + return _servletChannel.getHttpInput().getReadListenerInvocationType(); } /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index 2da49fc12de4..59562a372c80 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -31,7 +31,7 @@ *

While this class is-a Runnable, it should never be dispatched in it's own thread. It is a runnable only so that the calling thread can use {@link * Context#run(Runnable)} to setup classloaders etc.

*/ -public class HttpInput extends ServletInputStream implements Invocable +public class HttpInput extends ServletInputStream { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); @@ -160,8 +160,7 @@ public boolean isAsync() return _readListener != null; } - @Override - public Invocable.InvocationType getInvocationType() + public Invocable.InvocationType getReadListenerInvocationType() { // This is the invocation type used for demand callbacks. // If we are blocking mode, then we implement the callbacks, which just wake up the blocked application thread diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index cfa6f3ff2d5c..0857f406e33e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -545,7 +545,7 @@ public void handle() case WRITE_CALLBACK: { - _context.run(() -> _servletContextRequest.getHttpOutput().writeCallback()); + _context.run(_servletContextRequest.getHttpOutput()::writeCallback); break; } From d2694962dbe5ff9b627e5981d9498d730f167699 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 11:19:18 +0200 Subject: [PATCH 13/27] cleanup core fix Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/server/handler/ContextRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java index 01d437a5e0df..a86ccce5fdbe 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java @@ -59,7 +59,7 @@ public Context getContext() return _context; } - private class OnContextDemand implements Runnable, Invocable + private class OnContextDemand implements Invocable.Task { private final Runnable _demandCallback; From 8031d7c78a5037c2a057a0e5ec54a2f2d83d2f4d Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 11:21:26 +0200 Subject: [PATCH 14/27] remove redundant test Signed-off-by: Ludovic Orban --- .../jetty/server/ThreadStarvationTest.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java index 2a328db3711d..db57ba38ceba 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ThreadStarvationTest.java @@ -161,34 +161,6 @@ public void dispose() throws Exception } } - @ParameterizedTest - @MethodSource("scenarios") - public void testReadInput(Scenario scenario) throws Exception - { - prepareServer(scenario, new ReadHandler()).start(); - - try (Socket client = scenario.clientSocketProvider.newSocket("localhost", _connector.getLocalPort())) - { - client.setSoTimeout(10000); - OutputStream os = client.getOutputStream(); - InputStream is = client.getInputStream(); - - String request = - "GET / HTTP/1.0\r\n" + - "Host: localhost\r\n" + - "Content-Length: 10\r\n" + - "\r\n" + - "0123456789\r\n"; - os.write(request.getBytes(StandardCharsets.UTF_8)); - os.flush(); - - String response = IO.toString(is); - assertEquals(-1, is.read()); - assertThat(response, containsString("200 OK")); - assertThat(response, containsString("Read Input 10")); - } - } - @ParameterizedTest @MethodSource("scenarios") public void testReadStarvation(Scenario scenario) throws Exception From f95b9cf8a976bae4aaceff74f2acae96dd95e6ca Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 12:46:41 +0200 Subject: [PATCH 15/27] use inner classes for invocable tasks Signed-off-by: Ludovic Orban --- .../io/internal/ContentSourceString.java | 20 +++++++- .../jetty/server/handler/ContextRequest.java | 8 +-- .../ee10/servlet/AsyncContentProducer.java | 51 ++++++++++--------- .../eclipse/jetty/ee9/nested/HttpChannel.java | 11 ++-- 4 files changed, 56 insertions(+), 34 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java index 8c795437a619..29627e971644 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceString.java @@ -25,12 +25,15 @@ public class ContentSourceString private final Content.Source content; private final CharsetStringBuilder text; private final Promise promise; + private final ConvertInvocableTask convertInvocableTask; public ContentSourceString(Content.Source content, Charset charset, Promise promise) { this.content = content; this.text = CharsetStringBuilder.forCharset(charset); this.promise = promise; + // Inner class used instead of lambda for clarity in stack traces. + this.convertInvocableTask = new ConvertInvocableTask(); } public void convert() @@ -40,7 +43,7 @@ public void convert() Content.Chunk chunk = content.read(); if (chunk == null) { - content.demand(Invocable.from(Invocable.getInvocationType(promise), this::convert)); + content.demand(convertInvocableTask); return; } if (Content.Chunk.isFailure(chunk)) @@ -72,4 +75,19 @@ private void succeed() promise.failed(x); } } + + private class ConvertInvocableTask implements Invocable.Task + { + @Override + public void run() + { + convert(); + } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(promise); + } + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java index a86ccce5fdbe..3a73222d4001 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextRequest.java @@ -37,8 +37,8 @@ protected ContextRequest(ContextHandler.ScopedContext context, Request request) @Override public void demand(Runnable demandCallback) { - // inner class used instead of lambda for clarity in stack traces - super.demand(new OnContextDemand(demandCallback)); + // Inner class used instead of lambda for clarity in stack traces. + super.demand(new OnContextDemandInvocableTask(demandCallback)); } @Override @@ -59,11 +59,11 @@ public Context getContext() return _context; } - private class OnContextDemand implements Invocable.Task + private class OnContextDemandInvocableTask implements Invocable.Task { private final Runnable _demandCallback; - public OnContextDemand(Runnable demandCallback) + private OnContextDemandInvocableTask(Runnable demandCallback) { _demandCallback = demandCallback; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index 37897ad10faf..954dfeac2ef9 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -31,13 +31,14 @@ * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextChunk()} will never block * but will return null when there is no available content. */ -class AsyncContentProducer implements ContentProducer, Invocable.Task +class AsyncContentProducer implements ContentProducer { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final Content.Chunk RECYCLED_ERROR_CHUNK = Content.Chunk.from(new StaticException("ContentProducer has been recycled"), true); final AutoLock _lock; private final ServletChannel _servletChannel; + private final IsReadyInvocableTask _isReadyInvocableTask; private Content.Chunk _chunk; private long _firstByteNanoTime = Long.MIN_VALUE; private long _bytesArrived; @@ -50,6 +51,8 @@ class AsyncContentProducer implements ContentProducer, Invocable.Task { _servletChannel = servletChannel; _lock = lock; + // Inner class used instead of lambda for clarity in stack traces. + _isReadyInvocableTask = new IsReadyInvocableTask(); } ServletChannel getServletChannel() @@ -251,7 +254,7 @@ public boolean isReady() } state.onReadUnready(); - _servletChannel.getRequest().demand(this); + _servletChannel.getRequest().demand(_isReadyInvocableTask); if (LOG.isDebugEnabled()) LOG.debug("isReady(), no chunk {}", this); @@ -263,28 +266,6 @@ boolean isUnready() return _servletChannel.getServletRequestState().isInputUnready(); } - /** - * This run method is used as the Runnable for demand - */ - @Override - public void run() - { - if (LOG.isDebugEnabled()) - LOG.debug("isReady() demand callback {}", this); - // We could call this.onContentProducible() directly but this - // would mean we would need to take the lock here while it - // is the responsibility of the HttpInput to take it. - if (_servletChannel.getHttpInput().onContentProducible()) - _servletChannel.handle(); - } - - @Override - public InvocationType getInvocationType() - { - // This is the invocation type when the producer is passed as demand, so ask the HttpInput. - return _servletChannel.getHttpInput().getReadListenerInvocationType(); - } - /** * Never returns an empty chunk that isn't a failure and/or last. */ @@ -420,4 +401,26 @@ public String toString() return getClass().getSimpleName() + " permits=" + _permits; } } + + private class IsReadyInvocableTask implements Invocable.Task + { + @Override + public void run() + { + if (LOG.isDebugEnabled()) + LOG.debug("isReady() demand callback {}", this); + // We could call this.onContentProducible() directly but this + // would mean we would need to take the lock here while it + // is the responsibility of the HttpInput to take it. + if (_servletChannel.getHttpInput().onContentProducible()) + _servletChannel.handle(); + } + + @Override + public InvocationType getInvocationType() + { + // This is the invocation type when the producer is passed as demand, so ask the HttpInput. + return _servletChannel.getHttpInput().getReadListenerInvocationType(); + } + } } 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 5e1f6cbeb6ed..1c467dd4ebfc 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 @@ -115,6 +115,7 @@ public HttpChannel(ContextHandler contextHandler, ConnectionMetaData connectionM _combinedListener = new HttpChannelListeners(_connector.getBeans(Listener.class)); _requestDispatcher = new RequestDispatchable(); _asyncDispatcher = new AsyncDispatchable(); + // Inner class used instead of lambda for clarity in stack traces. _needContentTask = new NeedContentInvocableTask(); if (LOG.isDebugEnabled()) @@ -1605,16 +1606,16 @@ public void dispatch() throws IOException, ServletException private class NeedContentInvocableTask implements Invocable.Task { @Override - public InvocationType getInvocationType() + public void run() { - return getRequest().getHttpInput().getReadListenerInvocationType(); + if (getRequest().getHttpInput().onContentProducible()) + handle(); } @Override - public void run() + public InvocationType getInvocationType() { - if (getRequest().getHttpInput().onContentProducible()) - handle(); + return getRequest().getHttpInput().getReadListenerInvocationType(); } } } From a0f1e39630c39c21a873be221591b7382382787e Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 15:22:54 +0200 Subject: [PATCH 16/27] use inner classes for invocable tasks Signed-off-by: Ludovic Orban --- .../io/content/ContentSourceTransformer.java | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java index 66f75502e4d5..c0619e25ea88 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceTransformer.java @@ -100,7 +100,8 @@ public void demand(Runnable demandCallback) { this.demandCallback = Objects.requireNonNull(demandCallback); if (needsRawRead) - rawSource.demand(Invocable.from(Invocable.getInvocationType(demandCallback), () -> invoker.run(this::invokeDemandCallback))); + // Inner class used instead of lambda for clarity in stack traces. + rawSource.demand(new DemandInvocableTask(demandCallback, this::invokeDemandCallback)); else invoker.run(this::invokeDemandCallback); } @@ -158,4 +159,28 @@ private Content.Chunk process(Content.Chunk rawChunk) * @return a transformed chunk or {@code null} */ protected abstract Content.Chunk transform(Content.Chunk inputChunk); + + private class DemandInvocableTask implements Invocable.Task + { + private final Runnable demandCallback; + private final Runnable invokeDemandCallback; + + private DemandInvocableTask(Runnable demandCallback, Runnable invokeDemandCallback) + { + this.demandCallback = demandCallback; + this.invokeDemandCallback = invokeDemandCallback; + } + + @Override + public void run() + { + invoker.run(invokeDemandCallback); + } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(demandCallback); + } + } } From bbda85d68530f479b9b8d0887a6213e32dfbf03e Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 18 Oct 2024 16:08:03 +0200 Subject: [PATCH 17/27] review usages of CS.demand() w.r.t InvocationType Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/client/Response.java | 1 + .../jetty/client/transport/HttpSender.java | 3 +- .../client/transport/ResponseListeners.java | 28 ++++++++++++-- .../org/eclipse/jetty/http/MultiPart.java | 37 +++++++++++++++---- .../eclipse/jetty/io/ChunkAccumulator.java | 13 ++++++- .../jetty/io/internal/ContentCopier.java | 3 +- .../io/internal/ContentSourceByteBuffer.java | 9 ++++- .../jetty/server/handler/DelayedHandler.java | 3 +- .../server/handler/ThreadLimitHandler.java | 26 ++++++++++++- .../transport/HttpClientDemandTest.java | 9 ++++- 10 files changed, 113 insertions(+), 19 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Response.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Response.java index c07188acd0fa..a9dc31084430 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Response.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Response.java @@ -188,6 +188,7 @@ interface AsyncContentListener extends ContentSourceListener default void onContentSource(Response response, Content.Source contentSource) { Content.Chunk chunk = contentSource.read(); + // demandCallback eventually calls onContent() which calls end-user code, so its InvocationType must be BLOCKING. Runnable demandCallback = () -> onContentSource(response, contentSource); if (chunk == null) { diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java index 69bade58dd68..b1135560fb8e 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IteratingCallback; import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -543,7 +544,7 @@ protected Action process() throws Throwable // No content after the headers, demand. demanded = true; assert content != null; - content.demand(this::succeeded); + content.demand(Invocable.from(getInvocationType(), this::succeeded)); return Action.SCHEDULED; } else diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/ResponseListeners.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/ResponseListeners.java index de70d3b29fd5..75b1efc88027 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/ResponseListeners.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/ResponseListeners.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.io.content.ByteBufferContentSource; import org.eclipse.jetty.util.ExceptionUtil; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -231,7 +232,7 @@ private static void consume(Content.Source contentSource) if (chunk != null) chunk.release(); if (chunk == null || !chunk.isLast()) - contentSource.demand(() -> consume(contentSource)); + contentSource.demand(Invocable.from(Invocable.InvocationType.NON_BLOCKING, () -> consume(contentSource))); } private static void notifyContentSource(Response.ContentSourceListener listener, Response response, Content.Source contentSource) @@ -490,7 +491,7 @@ private void onDemandCallback() { // Retry the demand on spurious wakeup to avoid passing // a null chunk to the demultiplexer's ContentSources. - originalContentSource.demand(this::onDemandCallback); + originalContentSource.demand(Invocable.from(deriveOriginalContentSourcesInvocationType(), this::onDemandCallback)); return; } // Demultiplexer content sources are invoked sequentially to be consistent with other listeners, @@ -502,6 +503,20 @@ private void onDemandCallback() chunk.release(); } + private Invocable.InvocationType deriveOriginalContentSourcesInvocationType() + { + Invocable.InvocationType invocationType = null; + for (ContentSource contentSource : contentSources) + { + Invocable.InvocationType demandCallbackInvocationType = contentSource.getDemandCallbackInvocationType(); + if (invocationType == null) + invocationType = demandCallbackInvocationType; + else + invocationType = Invocable.combine(invocationType, demandCallbackInvocationType); + } + return invocationType == null ? Invocable.InvocationType.NON_BLOCKING : invocationType; + } + private void registerFailure(ContentSource contentSource, Throwable failure) { boolean processFail = false; @@ -524,7 +539,7 @@ else if (counters.total() == listeners.size()) if (processFail) originalContentSource.fail(failure); else if (processDemand) - originalContentSource.demand(this::onDemandCallback); + originalContentSource.demand(Invocable.from(deriveOriginalContentSourcesInvocationType(), this::onDemandCallback)); if (LOG.isDebugEnabled()) LOG.debug("Registered failure on {}; {}", contentSource, counters); @@ -547,7 +562,7 @@ private void registerDemand(ContentSource contentSource) } } if (processDemand) - originalContentSource.demand(this::onDemandCallback); + originalContentSource.demand(Invocable.from(deriveOriginalContentSourcesInvocationType(), this::onDemandCallback)); if (LOG.isDebugEnabled()) LOG.debug("Registered demand on {}; {}", contentSource, counters); @@ -641,6 +656,11 @@ private void onDemandCallback() } } + private Invocable.InvocationType getDemandCallbackInvocationType() + { + return Invocable.getInvocationType(demandCallbackRef.get()); + } + @Override public Content.Chunk read() { diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index fd9d89673f34..82ca64c79d7e 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -45,6 +45,7 @@ import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Utf8StringBuilder; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.eclipse.jetty.util.thread.SerializedInvoker; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -823,14 +824,8 @@ public void demand(Runnable demandCallback) } if (part != null) { - part.getContentSource().demand(() -> - { - try (AutoLock ignoredAgain = lock.lock()) - { - this.demand = null; - } - demandCallback.run(); - }); + // Inner class used instead of lambda for clarity in stack traces. + part.getContentSource().demand(new DemandInvocableTask(demandCallback)); } else if (invoke) { @@ -887,6 +882,32 @@ private enum State { FIRST, MIDDLE, HEADERS, CONTENT, COMPLETE } + + private class DemandInvocableTask implements Invocable.Task + { + private final Runnable demandCallback; + + private DemandInvocableTask(Runnable demandCallback) + { + this.demandCallback = demandCallback; + } + + @Override + public void run() + { + try (AutoLock ignoredAgain = lock.lock()) + { + AbstractContentSource.this.demand = null; + } + demandCallback.run(); + } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(demandCallback); + } + } } /** diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java index c0c37eb34111..e5a390e15cae 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ChunkAccumulator.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.io.Content.Chunk; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.CompletableTask; +import org.eclipse.jetty.util.thread.Invocable; /** * An accumulator of {@link Content.Chunk}s used to facilitate minimal copy @@ -164,7 +165,7 @@ protected RetainableByteBuffer take(ChunkAccumulator accumulator) return task.start(); } - private abstract static class AccumulatorTask extends CompletableTask + private abstract static class AccumulatorTask extends CompletableTask implements Invocable { private final Content.Source _source; private final ChunkAccumulator _accumulator = new ChunkAccumulator(); @@ -222,6 +223,16 @@ public void run() } } + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + + /** + * Implementations must be {@link InvocationType#NON_BLOCKING non-blocking}, + * or {@link #getInvocationType()} must be overridden accordingly. + */ protected abstract T take(ChunkAccumulator accumulator); } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java index b4b59f851d4b..96641e653fa1 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentCopier.java @@ -17,6 +17,7 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.ExceptionUtil; import org.eclipse.jetty.util.IteratingNestedCallback; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -57,7 +58,7 @@ protected Action process() throws Throwable if (current == null) { - source.demand(this::succeeded); + source.demand(Invocable.from(getInvocationType(), this::succeeded)); return Action.SCHEDULED; } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java index 9db5923b287f..4c8c14dd0c75 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/internal/ContentSourceByteBuffer.java @@ -18,8 +18,9 @@ import org.eclipse.jetty.io.ByteBufferAccumulator; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.thread.Invocable; -public class ContentSourceByteBuffer implements Runnable +public class ContentSourceByteBuffer implements Invocable.Task { private final ByteBufferAccumulator accumulator = new ByteBufferAccumulator(); private final Content.Source source; @@ -62,4 +63,10 @@ public void run() } } } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(promise); + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index a978b054f202..2dc36973d2c8 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -32,6 +32,7 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.thread.Invocable; public class DelayedHandler extends Handler.Wrapper { @@ -180,7 +181,7 @@ protected void delay() Content.Chunk chunk = super.getRequest().read(); if (chunk == null) { - getRequest().demand(this::onContent); + getRequest().demand(Invocable.from(InvocationType.NON_BLOCKING, this::onContent)); } else { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ThreadLimitHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ThreadLimitHandler.java index 19cc269220f9..5d2099417a88 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ThreadLimitHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ThreadLimitHandler.java @@ -41,6 +41,7 @@ import org.eclipse.jetty.util.annotation.ManagedOperation; import org.eclipse.jetty.util.annotation.Name; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -341,7 +342,8 @@ public void demand(Runnable onContent) { if (!_onContent.compareAndSet(null, Objects.requireNonNull(onContent))) throw new IllegalStateException("Pending demand"); - super.demand(this::onContent); + // Inner class used instead of lambda for clarity in stack traces. + super.demand(new OnContentInvocableTask(onContent)); } private void onContent() @@ -365,6 +367,28 @@ private void onPermittedContent(Permit permit) permit.release(); } } + + private class OnContentInvocableTask implements Invocable.Task + { + private final Runnable runnable; + + private OnContentInvocableTask(Runnable runnable) + { + this.runnable = runnable; + } + + @Override + public void run() + { + onContent(); + } + + @Override + public InvocationType getInvocationType() + { + return Invocable.getInvocationType(runnable); + } + } } private static class LimitedResponse extends Response.Wrapper implements Callback diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientDemandTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientDemandTest.java index 4bbf288a1abd..2ba8d99ae9dc 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientDemandTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientDemandTest.java @@ -42,6 +42,7 @@ import org.eclipse.jetty.util.IteratingCallback; import org.eclipse.jetty.util.IteratingNestedCallback; import org.eclipse.jetty.util.NanoTime; +import org.eclipse.jetty.util.thread.Invocable; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; @@ -589,7 +590,7 @@ public void testReadDemandInSpawnedThread(Transport transport) throws Exception assertThat(accumulatedSize, is(totalBytes)); } - private static class Accumulator implements Runnable + private static class Accumulator implements Invocable.Task { private final Content.Source contentSource; private final List chunks; @@ -613,6 +614,12 @@ public void run() if (!chunk.isLast()) contentSource.demand(this); } + + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } } private static class TestHandler extends Handler.Abstract From 9a0b4bae9d3901f18b988b89d44b3946ebd894bd Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 21 Oct 2024 11:48:31 +1100 Subject: [PATCH 18/27] Make the InvocableCompletableFuture non-dynamic --- .../ContentSourceCompletableFuture.java | 6 + .../org/eclipse/jetty/server/FormFields.java | 58 +++++++-- .../jetty/server/handler/DelayedHandler.java | 5 +- .../eclipse/jetty/server/FormFieldsTest.java | 5 +- .../eclipse/jetty/util/thread/Invocable.java | 93 ++++++++++---- .../jetty/util/thread/InvocableTest.java | 120 ++++++++++++++++++ 6 files changed, 251 insertions(+), 36 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java index 81e38f41b1a4..8d68759dfbc9 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ContentSourceCompletableFuture.java @@ -59,6 +59,12 @@ public abstract class ContentSourceCompletableFuture extends Invocable.Invoca public ContentSourceCompletableFuture(Content.Source content) { + this(content, InvocationType.NON_BLOCKING); + } + + public ContentSourceCompletableFuture(Content.Source content, InvocationType invocationType) + { + super(invocationType); _content = content; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java index ed0ba41fcfc5..8980733b2377 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java @@ -109,7 +109,6 @@ else if (attr instanceof Fields fields) * using the classname as the attribute name, else the request is used * as a {@link Content.Source} from which to read the fields and set the attribute. * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - * @see #from(Content.Source, Attributes, Charset, int, int) */ public static CompletableFuture from(Request request) { @@ -118,6 +117,20 @@ public static CompletableFuture from(Request request) return from(request, maxFields, maxLength); } + /** + * Find or create a {@link FormFields} from a {@link Content.Source}. + * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, + * using the classname as the attribute name, else the request is used + * as a {@link Content.Source} from which to read the fields and set the attribute. + * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. + */ + public static CompletableFuture from(Request request, InvocationType invocationType) + { + int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_LENGTH_DEFAULT); + return from(request, invocationType, getFormEncodedCharset(request), maxFields, maxLength); + } + /** * Find or create a {@link FormFields} from a {@link Content.Source}. * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, @@ -125,7 +138,6 @@ public static CompletableFuture from(Request request) * as a {@link Content.Source} from which to read the fields and set the attribute. * @param charset the {@link Charset} to use for byte to string conversion. * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - * @see #from(Content.Source, Attributes, Charset, int, int) */ public static CompletableFuture from(Request request, Charset charset) { @@ -134,6 +146,21 @@ public static CompletableFuture from(Request request, Charset charset) return from(request, charset, maxFields, maxLength); } + /** + * Find or create a {@link FormFields} from a {@link Content.Source}. + * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, + * using the classname as the attribute name, else the request is used + * as a {@link Content.Source} from which to read the fields and set the attribute. + * @param charset the {@link Charset} to use for byte to string conversion. + * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. + */ + public static CompletableFuture from(Request request, InvocationType invocationType, Charset charset) + { + int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + return from(request, invocationType, charset, maxFields, maxLength); + } + /** * Find or create a {@link FormFields} from a {@link Content.Source}. * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, @@ -142,7 +169,6 @@ public static CompletableFuture from(Request request, Charset charset) * @param maxFields The maximum number of fields to be parsed * @param maxLength The maximum total size of the fields * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - * @see #from(Content.Source, Attributes, Charset, int, int) */ public static CompletableFuture from(Request request, int maxFields, int maxLength) { @@ -158,11 +184,25 @@ public static CompletableFuture from(Request request, int maxFields, int * @param maxFields The maximum number of fields to be parsed * @param maxLength The maximum total size of the fields * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - * @see #from(Content.Source, Attributes, Charset, int, int) */ public static CompletableFuture from(Request request, Charset charset, int maxFields, int maxLength) { - return from(request, request, charset, maxFields, maxLength); + return from(request, InvocationType.NON_BLOCKING, request, charset, maxFields, maxLength); + } + + /** + * Find or create a {@link FormFields} from a {@link Content.Source}. + * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, + * using the classname as the attribute name, else the request is used + * as a {@link Content.Source} from which to read the fields and set the attribute. + * @param charset the {@link Charset} to use for byte to string conversion. + * @param maxFields The maximum number of fields to be parsed + * @param maxLength The maximum total size of the fields + * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. + */ + public static CompletableFuture from(Request request, InvocationType invocationType, Charset charset, int maxFields, int maxLength) + { + return from(request, invocationType, request, charset, maxFields, maxLength); } /** @@ -176,7 +216,7 @@ public static CompletableFuture from(Request request, Charset charset, i * @param maxLength The maximum total size of the fields * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. */ - static CompletableFuture from(Content.Source source, Attributes attributes, Charset charset, int maxFields, int maxLength) + static CompletableFuture from(Content.Source source, InvocationType invocationType, Attributes attributes, Charset charset, int maxFields, int maxLength) { Object attr = attributes.getAttribute(FormFields.class.getName()); if (attr instanceof FormFields futureFormFields) @@ -187,7 +227,7 @@ else if (attr instanceof Fields fields) if (charset == null) return EMPTY; - FormFields futureFormFields = new FormFields(source, charset, maxFields, maxLength); + FormFields futureFormFields = new FormFields(source, invocationType, charset, maxFields, maxLength); attributes.setAttribute(FormFields.class.getName(), futureFormFields); futureFormFields.parse(); return futureFormFields; @@ -217,9 +257,9 @@ private static int getContextAttribute(Context context, String attribute, int de private int _percent = 0; private byte _percentCode; - private FormFields(Content.Source source, Charset charset, int maxFields, int maxSize) + private FormFields(Content.Source source, InvocationType invocationType, Charset charset, int maxFields, int maxSize) { - super(source); + super(source, invocationType); _maxFields = maxFields; _maxLength = maxSize; _builder = CharsetStringBuilder.forCharset(charset); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index 2dc36973d2c8..e0dbdeecdc61 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -241,12 +241,13 @@ public UntilFormDelayedProcess(Handler handler, Request wrapped, Response respon @Override protected void delay() { - CompletableFuture futureFormFields = FormFields.from(getRequest(), _charset); + CompletableFuture futureFormFields = FormFields.from(getRequest(), InvocationType.BLOCKING, _charset); // if we are done already, then we are still in the scope of the original process call and can // process directly, otherwise we must execute a call to process as we are within a serialized // demand callback. - futureFormFields.whenComplete(futureFormFields.isDone() ? this::process : this::executeProcess); + boolean done = futureFormFields.isDone(); + futureFormFields.whenComplete(done ? this::process : this::executeProcess); } private void process(Fields fields, Throwable x) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/FormFieldsTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/FormFieldsTest.java index 40b6ff53217b..c0d76c304ca6 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/FormFieldsTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/FormFieldsTest.java @@ -29,6 +29,7 @@ import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; import org.eclipse.jetty.util.FutureCallback; +import org.eclipse.jetty.util.thread.Invocable; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -72,7 +73,7 @@ public void testValidFormFields(List chunks, Charset charset, int maxFie { AsyncContent source = new AsyncContent(); Attributes attributes = new Attributes.Mapped(); - CompletableFuture futureFields = FormFields.from(source, attributes, charset, maxFields, maxLength); + CompletableFuture futureFields = FormFields.from(source, Invocable.InvocationType.NON_BLOCKING, attributes, charset, maxFields, maxLength); assertFalse(futureFields.isDone()); int last = chunks.size() - 1; @@ -133,7 +134,7 @@ public static Stream invalidData() public void testInvalidFormFields(List chunks, Charset charset, int maxFields, int maxLength, Class expectedException) { AsyncContent source = new AsyncContent(); - CompletableFuture futureFields = FormFields.from(source, new Attributes.Mapped(), charset, maxFields, maxLength); + CompletableFuture futureFields = FormFields.from(source, Invocable.InvocationType.NON_BLOCKING, new Attributes.Mapped(), charset, maxFields, maxLength); assertFalse(futureFields.isDone()); int last = chunks.size() - 1; for (int i = 0; i <= last; i++) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index a2a90a67059f..2fa064e97543 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -15,7 +15,9 @@ import java.util.Objects; import java.util.concurrent.CompletionStage; -import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; @@ -191,6 +193,16 @@ static InvocationType combine(InvocationType it1, InvocationType it2) return InvocationType.BLOCKING; } + static InvocationType combineTypes(InvocationType... it) + { + if (it == null || it.length == 0) + return InvocationType.BLOCKING; + InvocationType type = it[0]; + for (int i = 1; i < it.length; i++) + type = combine(type, it[i]); + return type; + } + /** * Get the invocation type of an Object. * @@ -254,111 +266,146 @@ public InvocationType getInvocationType() /** * An extension of {@link java.util.concurrent.CompletableFuture} that is an {@link Invocable}. - * The {@link InvocationType} is initially the type used in construction (default NON_BLOCKING). - * If a non async method is called, then the invocation type of any passed function is used. - * @param + * The {@link InvocationType} is the type passed in construction (default NON_BLOCKING). + * Methods on {@link java.util.concurrent.CompletableFuture} that may act in contradiction to the passed + * {@link InvocationType} are extended to throw {@link IllegalStateException} in those circumstances. + * @param The type of the result */ class InvocableCompletableFuture extends java.util.concurrent.CompletableFuture implements Invocable { - private final AtomicReference _invocationType = new AtomicReference<>(); + private final InvocationType _invocationType; - public InvocableCompletableFuture() + public InvocableCompletableFuture(InvocationType invocationType) { - this(null); + _invocationType = Objects.requireNonNull(invocationType); + if (_invocationType == InvocationType.EITHER) + throw new IllegalArgumentException("EITHER is not supported"); } - public InvocableCompletableFuture(InvocationType invocationType) + @Override + public InvocationType getInvocationType() { - _invocationType.set(Objects.requireNonNullElse(invocationType, InvocationType.NON_BLOCKING)); + return _invocationType; } @Override - public InvocationType getInvocationType() + public V get() throws InterruptedException, ExecutionException { - return _invocationType.get(); + if (getInvocationType() == InvocationType.BLOCKING && !isDone()) + throw new IllegalStateException("Must be NON_BLOCKING or completed"); + return super.get(); + } + + @Override + public V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException + { + if (getInvocationType() == InvocationType.BLOCKING && !isDone()) + throw new IllegalStateException("Must be NON_BLOCKING or completed"); + return super.get(timeout, unit); + } + + @Override + public V join() + { + if (!isDone() && getInvocationType() == InvocationType.BLOCKING && !isDone()) + throw new IllegalStateException("Must be NON_BLOCKING or completed"); + return super.join(); } @Override public java.util.concurrent.CompletableFuture acceptEither(CompletionStage other, Consumer action) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + if (!isDone() && getInvocationType() != combineTypes(getInvocationType(), Invocable.getInvocationType(other), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); + return super.acceptEither(other, action); } @Override public java.util.concurrent.CompletableFuture applyToEither(CompletionStage other, Function fn) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(fn))); + if (!isDone() && getInvocationType() != combineTypes(getInvocationType(), Invocable.getInvocationType(other), Invocable.getInvocationType(fn))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.applyToEither(other, fn); } @Override public java.util.concurrent.CompletableFuture handle(BiFunction fn) { - _invocationType.set(Invocable.getInvocationType(fn)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(fn))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.handle(fn); } @Override public java.util.concurrent.CompletableFuture runAfterBoth(CompletionStage other, Runnable action) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.runAfterBoth(other, action); } @Override public java.util.concurrent.CompletableFuture runAfterEither(CompletionStage other, Runnable action) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + if (!isDone() && getInvocationType() != combineTypes(getInvocationType(), Invocable.getInvocationType(other), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.runAfterEither(other, action); } @Override public java.util.concurrent.CompletableFuture thenAccept(Consumer action) { - _invocationType.set(Invocable.getInvocationType(action)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenAccept(action); } @Override public java.util.concurrent.CompletableFuture thenAcceptBoth(CompletionStage other, BiConsumer action) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(action))); + if (!isDone() && getInvocationType() != combineTypes(getInvocationType(), Invocable.getInvocationType(other), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenAcceptBoth(other, action); } @Override public java.util.concurrent.CompletableFuture thenApply(Function fn) { - _invocationType.set(Invocable.getInvocationType(fn)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(fn))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenApply(fn); } @Override public java.util.concurrent.CompletableFuture thenCombine(CompletionStage other, BiFunction fn) { - _invocationType.set(Invocable.combine(Invocable.getInvocationType(other), Invocable.getInvocationType(fn))); + if (!isDone() && getInvocationType() != combineTypes(getInvocationType(), Invocable.getInvocationType(other), Invocable.getInvocationType(fn))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenCombine(other, fn); } @Override public java.util.concurrent.CompletableFuture thenCompose(Function> fn) { - _invocationType.set(Invocable.getInvocationType(fn)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(fn))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenCompose(fn); } @Override public java.util.concurrent.CompletableFuture thenRun(Runnable action) { - _invocationType.set(Invocable.getInvocationType(action)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.thenRun(action); } @Override public java.util.concurrent.CompletableFuture whenComplete(BiConsumer action) { - _invocationType.set(Invocable.getInvocationType(action)); + if (!isDone() && getInvocationType() != combine(getInvocationType(), Invocable.getInvocationType(action))) + throw new IllegalStateException("Bad invocation type when not completed"); return super.whenComplete(action); } } diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/thread/InvocableTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/thread/InvocableTest.java index c8d81878deb6..d8f76a591b2e 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/thread/InvocableTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/thread/InvocableTest.java @@ -14,7 +14,11 @@ package org.eclipse.jetty.util.thread; import java.util.Queue; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import org.junit.jupiter.api.Test; @@ -26,6 +30,8 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class InvocableTest { @@ -51,6 +57,41 @@ public void testCombineType() assertThat(Invocable.combine(EITHER, BLOCKING), is(BLOCKING)); assertThat(Invocable.combine(EITHER, NON_BLOCKING), is(NON_BLOCKING)); assertThat(Invocable.combine(EITHER, EITHER), is(EITHER)); + + assertThat(Invocable.combineTypes(null, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(null, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(null, NON_BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(null, EITHER), is(BLOCKING)); + + assertThat(Invocable.combineTypes(BLOCKING, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, NON_BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, EITHER), is(BLOCKING)); + + assertThat(Invocable.combineTypes(NON_BLOCKING, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, NON_BLOCKING), is(NON_BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, EITHER), is(NON_BLOCKING)); + + assertThat(Invocable.combineTypes(EITHER, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, NON_BLOCKING), is(NON_BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, EITHER), is(EITHER)); + + assertThat(Invocable.combineTypes(EITHER, EITHER, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, EITHER, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, EITHER, NON_BLOCKING), is(NON_BLOCKING)); + assertThat(Invocable.combineTypes(EITHER, EITHER, EITHER), is(EITHER)); + + assertThat(Invocable.combineTypes(BLOCKING, EITHER, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, EITHER, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, EITHER, NON_BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(BLOCKING, EITHER, EITHER), is(BLOCKING)); + + assertThat(Invocable.combineTypes(NON_BLOCKING, EITHER, null), is(BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, EITHER, BLOCKING), is(BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, EITHER, NON_BLOCKING), is(NON_BLOCKING)); + assertThat(Invocable.combineTypes(NON_BLOCKING, EITHER, EITHER), is(NON_BLOCKING)); } @Test @@ -80,4 +121,83 @@ public void testCombineRunnable() r123.run(); assertThat(history, contains("R1", "R2", "R3")); } + + @Test + public void testBlockingInvocableCompletableFuture() throws Exception + { + CompletableFuture future = new Invocable.InvocableCompletableFuture<>(BLOCKING); + + // We can block in a passed function + CountDownLatch inFunction = new CountDownLatch(1); + CountDownLatch blockInFunction = new CountDownLatch(1); + future.thenRun(() -> + { + try + { + inFunction.countDown(); + assertTrue(blockInFunction.await(5, TimeUnit.SECONDS)); + } + catch (InterruptedException e) + { + throw new RuntimeException(e); + } + }); + + // We can use the functional APIs with blocking and unblocking callbacks + CountDownLatch thenRun = new CountDownLatch(2); + future.thenRun(thenRun::countDown); + future.thenRun(Invocable.from(NON_BLOCKING, thenRun::countDown)); + + // Since the future is blocking, we cannot use the blocking APIs else we risk deadlock + assertThrows(IllegalStateException.class, future::get); + assertThrows(IllegalStateException.class, future::join); + + // The invocation type is blocking + assertThat(Invocable.getInvocationType(future), is(BLOCKING)); + + // Completion thread calls functions and blocks there + Thread completionThread = new Thread(() -> future.complete("result")); + completionThread.start(); + assertTrue(inFunction.await(1, TimeUnit.SECONDS)); + completionThread.join(100); + assertTrue(completionThread.isAlive()); + assertTrue(future.isDone()); + + // let functions run to completion + blockInFunction.countDown(); + assertTrue(thenRun.await(5, TimeUnit.SECONDS)); + + // We can now use the blocking APIs, as we know they will not actually block + assertThat(future.get(), is("result")); + } + + @Test + public void testNonBlockingInvocableCompletableFuture() throws Exception + { + CompletableFuture future = new Invocable.InvocableCompletableFuture<>(NON_BLOCKING); + + // We cannot pass blocking functions + CountDownLatch thenRun = new CountDownLatch(2); + assertThrows(IllegalStateException.class, () -> future.thenRun(thenRun::countDown)); + + // But we can pass non-blocking functions + future.thenRun(Invocable.from(NON_BLOCKING, thenRun::countDown)); + + // We can use the blocking APIs, as any wake ups are non blocking. + assertThrows(TimeoutException.class, () -> future.get(10, TimeUnit.MILLISECONDS)); + + // The invocation type is non blocking + assertThat(Invocable.getInvocationType(future), is(NON_BLOCKING)); + + // completion does not block + future.complete("result"); + assertTrue(future.isDone()); + + // We can still use the blocking APIs + assertThat(future.get(), is("result")); + + // And we can now pass a blocking function + future.thenRun(thenRun::countDown); + assertTrue(thenRun.await(5, TimeUnit.SECONDS)); + } } From 18ab56d0fb82be03157e1087c54f8985c5c60cea Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 23 Oct 2024 06:36:51 +1100 Subject: [PATCH 19/27] Fragile linking of InvocableCompletableFutures --- .../eclipse/jetty/http/MultiPartFormData.java | 24 +++++- .../jetty/ee10/servlet/EagerFormHandler.java | 38 +++++---- .../servlet/ServletMultiPartFormData.java | 83 +++++++++++++++---- 3 files changed, 113 insertions(+), 32 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 69293958022f..7ee0a98f0636 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -34,6 +34,7 @@ import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable.InvocationType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -88,6 +89,20 @@ private MultiPartFormData() * @return the future parts */ public static CompletableFuture from(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config) + { + return from(content, InvocationType.NON_BLOCKING, attributes, contentType, config); + } + + /** + * Returns {@code multipart/form-data} parts using the given {@link Content.Source} and {@link MultiPartConfig}. + * + * @param content the source of the multipart content. + * @param attributes the attributes where the futureParts are tracked. + * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. + * @param config the multipart configuration. + * @return the future parts + */ + public static CompletableFuture from(Content.Source content, InvocationType invocationType, Attributes attributes, String contentType, MultiPartConfig config) { // Look for an existing future (we use the future here rather than the parts as it can remember any failure). CompletableFuture futureParts = MultiPartFormData.get(attributes); @@ -106,7 +121,7 @@ public static CompletableFuture from(Content.Source con Parser parser = new Parser(boundary); parser.configure(config); - futureParts = parser.parse(content); + futureParts = parser.parse(content, invocationType); attributes.setAttribute(MultiPartFormData.class.getName(), futureParts); return futureParts; } @@ -297,7 +312,12 @@ public Parser(String boundary, MultiPartCompliance multiPartCompliance, Complian public CompletableFuture parse(Content.Source content) { - ContentSourceCompletableFuture futureParts = new ContentSourceCompletableFuture<>(content) + return parse(content, InvocationType.NON_BLOCKING); + } + + public CompletableFuture parse(Content.Source content, InvocationType invocationType) + { + ContentSourceCompletableFuture futureParts = new ContentSourceCompletableFuture<>(content, invocationType) { @Override protected Parts parse(Content.Chunk chunk) throws Throwable diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index 015d051edda4..dfea1e06afc4 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -55,29 +55,37 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons CompletableFuture future = switch (mimeType) { - case FORM_ENCODED -> FormFields.from(request); - case MULTIPART_FORM_DATA -> ServletMultiPartFormData.from(Request.as(request, ServletContextRequest.class).getServletApiRequest(), contentType); + case FORM_ENCODED -> FormFields.from(request, InvocationType.BLOCKING); + case MULTIPART_FORM_DATA -> ServletMultiPartFormData.from(Request.as(request, ServletContextRequest.class).getServletApiRequest(), InvocationType.BLOCKING, contentType); default -> null; }; if (future == null) return super.handle(request, response, callback); - future.whenComplete((result, failure) -> + if (future.isDone()) { - // The result and failure are not handled here. Rather we call the next handler - // to allow the normal processing to handle the result or failure, which will be - // provided via the attribute to ServletApiRequest#getParts() - try - { - if (!super.handle(request, response, callback)) - callback.failed(new IllegalStateException("Not Handled")); - } - catch (Throwable x) + if (!super.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + else + { + future.whenComplete((result, failure) -> { - callback.failed(x); - } - }); + // The result and failure are not handled here. Rather we call the next handler + // to allow the normal processing to handle the result or failure, which will be + // provided via the attribute to ServletApiRequest#getParts() + try + { + if (!super.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + catch (Throwable x) + { + callback.failed(x); + } + }); + } return true; } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index b27b355b2cbb..d9ee03fb3c1e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.function.Function; import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletRequest; @@ -40,6 +41,7 @@ import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.thread.Invocable; /** *

Servlet specific class for multipart content support.

@@ -59,7 +61,18 @@ public class ServletMultiPartFormData */ public static CompletableFuture from(ServletRequest servletRequest) { - return from(servletRequest, servletRequest.getContentType()); + return from(servletRequest, Invocable.InvocationType.NON_BLOCKING, servletRequest.getContentType()); + } + + /** + * Get future {@link ServletMultiPartFormData.Parts} from a servlet request. + * @param servletRequest A servlet request + * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. + * @see #from(ServletRequest, String) + */ + public static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType) + { + return from(servletRequest, invocationType, servletRequest.getContentType()); } /** @@ -69,6 +82,17 @@ public static CompletableFuture from(ServletRequest servletRequest) * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. */ public static CompletableFuture from(ServletRequest servletRequest, String contentType) + { + return from(servletRequest, Invocable.InvocationType.NON_BLOCKING, contentType); + } + + /** + * Get future {@link ServletMultiPartFormData.Parts} from a servlet request. + * @param servletRequest A servlet request + * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. + * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. + */ + public static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType, String contentType) { // Look for an existing future (we use the future here rather than the parts as it can remember any failure). @SuppressWarnings("unchecked") @@ -102,11 +126,11 @@ public static CompletableFuture from(ServletRequest servletRequest, Strin ? servletContextRequest.getContext().getTempDirectory().toPath() : new File(config.getLocation()).toPath(); - // Look for an existing future MultiPartFormData.Parts - CompletableFuture futureFormData = MultiPartFormData.get(servletContextRequest); - if (futureFormData == null) + try { - try + // Look for an existing future MultiPartFormData.Parts + CompletableFuture futureFormData = MultiPartFormData.get(servletContextRequest); + if (futureFormData == null) { // No existing core parts, so we need to configure the parser. ServletContextHandler contextHandler = servletContextRequest.getServletContext().getServletContextHandler(); @@ -135,23 +159,52 @@ public static CompletableFuture from(ServletRequest servletRequest, Strin .maxSize(config.getMaxRequestSize()) .build(); - futureFormData = MultiPartFormData.from(source, servletContextRequest, contentType, multiPartConfig); - } - catch (Throwable failure) - { - return CompletableFuture.failedFuture(failure); + futureFormData = MultiPartFormData.from(source, invocationType, servletContextRequest, contentType, multiPartConfig); + } - } - // When available, convert the core parts to servlet parts - futureServletParts = futureFormData.thenApply(formDataParts -> new Parts(filesDirectory, formDataParts)); + // If we are already completed, ... + futureServletParts = (futureFormData.isDone()) + // we can just convert here + ? CompletableFuture.completedFuture(new Parts(filesDirectory, futureFormData.join())) + // Otherwise, when available, convert the core parts to servlet parts + : futureFormData.thenApply(new PartsFunction(invocationType, filesDirectory)); - // cache the result in attributes. - servletRequest.setAttribute(ServletMultiPartFormData.class.getName(), futureServletParts); + // cache the result in attributes. + servletRequest.setAttribute(ServletMultiPartFormData.class.getName(), futureServletParts); + } + catch (Throwable failure) + { + return CompletableFuture.failedFuture(failure); + } } return futureServletParts; } + private static class PartsFunction implements Function, Invocable + { + private final InvocationType _invocationType; + private final Path _filesDirectory; + + PartsFunction(InvocationType invocationType, Path filesDirectory) + { + _invocationType = invocationType; + _filesDirectory = filesDirectory; + } + + @Override + public InvocationType getInvocationType() + { + return _invocationType; + } + + @Override + public Parts apply(MultiPartFormData.Parts parts) + { + return new Parts(_filesDirectory, parts); + } + } + /** *

An ordered list of {@link Part}s that can be accessed by name.

*/ From b1c8252df53b2baf709737811c29888588a8f4b1 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 23 Oct 2024 06:41:53 +1100 Subject: [PATCH 20/27] javadoc --- .../main/java/org/eclipse/jetty/util/thread/Invocable.java | 4 ++++ .../java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java | 1 + .../eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java | 2 ++ 3 files changed, 7 insertions(+) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index 2fa064e97543..232cd677c1b7 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -269,6 +269,10 @@ public InvocationType getInvocationType() * The {@link InvocationType} is the type passed in construction (default NON_BLOCKING). * Methods on {@link java.util.concurrent.CompletableFuture} that may act in contradiction to the passed * {@link InvocationType} are extended to throw {@link IllegalStateException} in those circumstances. + *

+ * Counterintuitively, if the blocking APIs like {@link #get()} are to be used, the {@link InvocationType} + * should be {@link InvocationType#NON_BLOCKING}, as the wake-up callbacks used will not block. + * * @param The type of the result */ class InvocableCompletableFuture extends java.util.concurrent.CompletableFuture implements Invocable diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index dfea1e06afc4..3b1cf33f99c1 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -32,6 +32,7 @@ */ public class EagerFormHandler extends Handler.Wrapper { + // TODO replace with DelayedDispatchHandler public EagerFormHandler() { this(null); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index d9ee03fb3c1e..34935490bf23 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -67,6 +67,7 @@ public static CompletableFuture from(ServletRequest servletRequest) /** * Get future {@link ServletMultiPartFormData.Parts} from a servlet request. * @param servletRequest A servlet request + * @param invocationType The invocation type of the resulting CompletableFuture. * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. * @see #from(ServletRequest, String) */ @@ -89,6 +90,7 @@ public static CompletableFuture from(ServletRequest servletRequest, Strin /** * Get future {@link ServletMultiPartFormData.Parts} from a servlet request. * @param servletRequest A servlet request + * @param invocationType The invocation type of the resulting CompletableFuture. * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. */ From 6b0843581d38bc0b2c4ce46834d973727c299c5c Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 23 Oct 2024 10:40:20 +1100 Subject: [PATCH 21/27] Deprecated the CF APIs and replaced with explicit getXxx onXxx methods --- .../eclipse/jetty/http/MultiPartFormData.java | 66 ++++++++- .../authentication/FormAuthenticator.java | 27 ++-- .../org/eclipse/jetty/server/FormFields.java | 134 +++++++++++++----- .../jetty/server/handler/DelayedHandler.java | 57 ++++++-- .../server/handler/DelayedHandlerTest.java | 73 ++++++++++ .../eclipse/jetty/util/thread/Invocable.java | 25 ++++ .../jetty/ee10/servlet/EagerFormHandler.java | 105 ++++++++++---- .../jetty/ee10/servlet/ServletApiRequest.java | 16 +-- .../servlet/ServletMultiPartFormData.java | 55 ++++++- 9 files changed, 449 insertions(+), 109 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 7ee0a98f0636..cbe5af83e618 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; import java.util.function.Function; import org.eclipse.jetty.io.Content; @@ -34,6 +35,7 @@ import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.AutoLock; +import org.eclipse.jetty.util.thread.Invocable; import org.eclipse.jetty.util.thread.Invocable.InvocationType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -79,6 +81,53 @@ private MultiPartFormData() { } + /** + * Returns {@code multipart/form-data} parts using the given {@link Content.Source} and {@link MultiPartConfig}, + * blocking if necessary. + * + * @param content the source of the multipart content. + * @param attributes the attributes where the futureParts are tracked. + * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. + * @param config the multipart configuration. + * @return the parts + */ + public static MultiPartFormData.Parts getParts(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config) + { + return from(content, InvocationType.NON_BLOCKING, attributes, contentType, config).join(); + } + + /** + * @param content the source of the multipart content. + * @param attributes the attributes where the futureParts are tracked. + * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. + * @param config the multipart configuration. + * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). + * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} + * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + */ + public static void onParts(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config, BiConsumer immediate, Invocable.InvocableBiConsumer future) + { + CompletableFuture futureParts = from(content, future.getInvocationType(), attributes, contentType, config); + if (futureParts.isDone()) + { + Parts parts = null; + Throwable error = null; + try + { + parts = futureParts.get(); + } + catch (Throwable t) + { + error = t; + } + immediate.accept(parts, error); + } + else + { + futureParts.whenComplete(future); + } + } + /** * Returns {@code multipart/form-data} parts using the given {@link Content.Source} and {@link MultiPartConfig}. * @@ -87,7 +136,10 @@ private MultiPartFormData() * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. * @param config the multipart configuration. * @return the future parts + * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config) { return from(content, InvocationType.NON_BLOCKING, attributes, contentType, config); @@ -101,7 +153,10 @@ public static CompletableFuture from(Content.Source con * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. * @param config the multipart configuration. * @return the future parts + * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Content.Source content, InvocationType invocationType, Attributes attributes, String contentType, MultiPartConfig config) { // Look for an existing future (we use the future here rather than the parts as it can remember any failure). @@ -130,9 +185,10 @@ public static CompletableFuture from(Content.Source con /** * Returns {@code multipart/form-data} parts using {@link MultiPartCompliance#RFC7578}. - * @deprecated use {@link #from(Content.Source, Attributes, String, MultiPartConfig)}. + * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} */ - @Deprecated + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Attributes attributes, String boundary, Function> parse) { return from(attributes, MultiPartCompliance.RFC7578, ComplianceViolation.Listener.NOOP, boundary, parse); @@ -147,9 +203,10 @@ public static CompletableFuture from(Attributes attributes, String bounda * @param boundary the boundary for the {@code multipart/form-data} parts * @param parse the parser completable future * @return the future parts - * @deprecated use {@link #from(Content.Source, Attributes, String, MultiPartConfig)}. + * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} */ - @Deprecated + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Attributes attributes, MultiPartCompliance compliance, ComplianceViolation.Listener listener, String boundary, Function> parse) { CompletableFuture futureParts = get(attributes); @@ -168,6 +225,7 @@ public static CompletableFuture from(Attributes attributes, MultiPartComp * @return the future parts */ @SuppressWarnings("unchecked") + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture get(Attributes attributes) { return (CompletableFuture)attributes.getAttribute(MultiPartFormData.class.getName()); diff --git a/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java index 79e2d5828917..864fa04cc885 100644 --- a/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java +++ b/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java @@ -13,8 +13,7 @@ package org.eclipse.jetty.security.authentication; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; +import java.util.concurrent.CompletionException; import java.util.function.Function; import org.eclipse.jetty.http.HttpHeader; @@ -202,8 +201,8 @@ public Request prepareRequest(Request request, AuthenticationState authenticatio session.removeAttribute(__J_URI); Object post = session.removeAttribute(__J_POST); - if (post instanceof CompletableFuture futureFields) - FormFields.set(request, (CompletableFuture)futureFields); + if (post instanceof Fields futureFields) + FormFields.set(request, futureFields); String method = (String)session.removeAttribute(__J_METHOD); if (method != null && request.getMethod().equals(method)) @@ -225,16 +224,9 @@ public String getMethod() protected Fields getParameters(Request request) { - try - { - Fields queryFields = Request.extractQueryParameters(request); - Fields formFields = FormFields.from(request).get(); - return Fields.combine(queryFields, formFields); - } - catch (InterruptedException | ExecutionException e) - { - throw new RuntimeException(e); - } + Fields queryFields = Request.extractQueryParameters(request); + Fields formFields = FormFields.getFields(request); + return Fields.combine(queryFields, formFields); } protected String encodeURL(String url, Request request) @@ -334,15 +326,14 @@ public AuthenticationState validateRequest(Request request, Response response, C { try { - CompletableFuture futureFields = FormFields.from(request); - futureFields.get(); + Fields futureFields = FormFields.getFields(request); session.setAttribute(__J_POST, futureFields); } - catch (ExecutionException e) + catch (CompletionException e) { throw new ServerAuthException(e.getCause()); } - catch (InterruptedException e) + catch (Exception e) { throw new ServerAuthException(e); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java index 8980733b2377..d24c08a2ed97 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java @@ -18,6 +18,7 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MimeTypes; @@ -82,17 +83,100 @@ public static Charset getFormEncodedCharset(Request request) * @param request The request to which to associate the fields with * @param fields A {@link CompletableFuture} that will provide either the fields or a failure. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static void set(Request request, CompletableFuture fields) { request.setAttribute(FormFields.class.getName(), fields); } + /** + * Set a {@link Fields} or related failure for the request + * @param request The request to which to associate the fields with + * @param fields A {@link CompletableFuture} that will provide either the fields or a failure. + */ + public static void set(Request request, Fields fields) + { + request.setAttribute(FormFields.class.getName(), fields); + } + + /** + * Get the Fields from a request, blocking if necessary. + * @param request The request to get the Fields from + * @return the Fields + */ + public static Fields getFields(Request request) + { + CompletableFuture fields = from(request, InvocationType.NON_BLOCKING); + return fields.join(); + } + + /** + * Get the Fields from a request, blocking if necessary. + * @param request The request to get the Fields from + * @param maxFields The maximum number of fields to accept + * @param maxLength The maximum length of fields + * @return the Fields + */ + public static Fields getFields(Request request, int maxFields, int maxLength) + { + CompletableFuture fields = from(request, InvocationType.NON_BLOCKING, getFormEncodedCharset(request), maxFields, maxLength); + return fields.join(); + } + + /** + * Actions to take when parsing FormFields asynchronously from a request is complete + * @param request The request to parse FormFields from + * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). + * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} + * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + */ + public static void onFields(Request request, BiConsumer immediate, InvocableBiConsumer future) + { + onFields(from(request, future.getInvocationType()), immediate, future); + } + + /** + * Actions to take when parsing FormFields asynchronously from a request is complete + * @param request The request to parse FormFields from + * @param charset The charset of the form. + * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). + * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} + * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + */ + public static void onFields(Request request, Charset charset, BiConsumer immediate, InvocableBiConsumer future) + { + onFields(from(request, future.getInvocationType(), charset), immediate, future); + } + + private static void onFields(CompletableFuture futureFields, BiConsumer immediate, InvocableBiConsumer future) + { + if (futureFields.isDone()) + { + Fields fields = null; + Throwable error = null; + try + { + fields = futureFields.get(); + } + catch (Throwable t) + { + error = t; + } + immediate.accept(fields, error); + } + else + { + futureFields.whenComplete(future); + } + } + /** * @param request The request to enquire from * @return A {@link CompletableFuture} that will provide either the fields or a failure, or null if none set. * @see #from(Request) * */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture get(Request request) { Object attr = request.getAttribute(FormFields.class.getName()); @@ -110,6 +194,7 @@ else if (attr instanceof Fields fields) * as a {@link Content.Source} from which to read the fields and set the attribute. * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Request request) { int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); @@ -117,14 +202,8 @@ public static CompletableFuture from(Request request) return from(request, maxFields, maxLength); } - /** - * Find or create a {@link FormFields} from a {@link Content.Source}. - * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, - * using the classname as the attribute name, else the request is used - * as a {@link Content.Source} from which to read the fields and set the attribute. - * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - */ - public static CompletableFuture from(Request request, InvocationType invocationType) + @Deprecated(forRemoval = true, since = "12.0.15") + private static CompletableFuture from(Request request, InvocationType invocationType) { int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_LENGTH_DEFAULT); @@ -139,6 +218,7 @@ public static CompletableFuture from(Request request, InvocationType inv * @param charset the {@link Charset} to use for byte to string conversion. * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Request request, Charset charset) { int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); @@ -146,15 +226,8 @@ public static CompletableFuture from(Request request, Charset charset) return from(request, charset, maxFields, maxLength); } - /** - * Find or create a {@link FormFields} from a {@link Content.Source}. - * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, - * using the classname as the attribute name, else the request is used - * as a {@link Content.Source} from which to read the fields and set the attribute. - * @param charset the {@link Charset} to use for byte to string conversion. - * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - */ - public static CompletableFuture from(Request request, InvocationType invocationType, Charset charset) + @Deprecated(forRemoval = true, since = "12.0.15") + private static CompletableFuture from(Request request, InvocationType invocationType, Charset charset) { int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); @@ -170,6 +243,7 @@ public static CompletableFuture from(Request request, InvocationType inv * @param maxLength The maximum total size of the fields * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Request request, int maxFields, int maxLength) { return from(request, getFormEncodedCharset(request), maxFields, maxLength); @@ -185,37 +259,19 @@ public static CompletableFuture from(Request request, int maxFields, int * @param maxLength The maximum total size of the fields * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Request request, Charset charset, int maxFields, int maxLength) { return from(request, InvocationType.NON_BLOCKING, request, charset, maxFields, maxLength); } - /** - * Find or create a {@link FormFields} from a {@link Content.Source}. - * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, - * using the classname as the attribute name, else the request is used - * as a {@link Content.Source} from which to read the fields and set the attribute. - * @param charset the {@link Charset} to use for byte to string conversion. - * @param maxFields The maximum number of fields to be parsed - * @param maxLength The maximum total size of the fields - * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - */ - public static CompletableFuture from(Request request, InvocationType invocationType, Charset charset, int maxFields, int maxLength) + @Deprecated(forRemoval = true, since = "12.0.15") + private static CompletableFuture from(Request request, InvocationType invocationType, Charset charset, int maxFields, int maxLength) { return from(request, invocationType, request, charset, maxFields, maxLength); } - /** - * Find or create a {@link FormFields} from a {@link Content.Source}. - * @param source The {@link Content.Source} from which to read the fields. - * @param attributes The {@link Attributes} in which to look for an existing {@link CompletableFuture} of - * {@link FormFields}, using the classname as the attribute name. If not found the attribute - * is set with the created {@link CompletableFuture} of {@link FormFields}. - * @param charset the {@link Charset} to use for byte to string conversion. - * @param maxFields The maximum number of fields to be parsed - * @param maxLength The maximum total size of the fields - * @return A {@link CompletableFuture} that will provide the {@link Fields} or a failure. - */ + @Deprecated(forRemoval = true, since = "12.0.15") static CompletableFuture from(Content.Source source, InvocationType invocationType, Attributes attributes, Charset charset, int maxFields, int maxLength) { Object attr = attributes.getAttribute(FormFields.class.getName()); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index e0dbdeecdc61..c5317e9dc378 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -16,7 +16,6 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.Objects; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.http.HttpField; @@ -24,6 +23,8 @@ import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.MultiPartConfig; +import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.FormFields; import org.eclipse.jetty.server.Handler; @@ -113,6 +114,14 @@ protected DelayedProcess newDelayedProcess(boolean contentExpected, String conte return switch (mimeType) { case FORM_ENCODED -> new UntilFormDelayedProcess(handler, request, response, callback, contentType); + case MULTIPART_FORM_DATA -> + { + if (request.getContext().getAttribute(MultiPartConfig.class.getName()) instanceof MultiPartConfig mpc) + yield new UntilMultipartDelayedProcess(handler, request, response, callback, contentType, mpc); + if (getServer().getAttribute(MultiPartConfig.class.getName()) instanceof MultiPartConfig mpc) + yield new UntilMultipartDelayedProcess(handler, request, response, callback, contentType, mpc); + yield null; + } default -> new UntilContentDelayedProcess(handler, request, response, callback); }; } @@ -241,13 +250,7 @@ public UntilFormDelayedProcess(Handler handler, Request wrapped, Response respon @Override protected void delay() { - CompletableFuture futureFormFields = FormFields.from(getRequest(), InvocationType.BLOCKING, _charset); - - // if we are done already, then we are still in the scope of the original process call and can - // process directly, otherwise we must execute a call to process as we are within a serialized - // demand callback. - boolean done = futureFormFields.isDone(); - futureFormFields.whenComplete(done ? this::process : this::executeProcess); + FormFields.onFields(getRequest(), _charset, this::process, Invocable.from(InvocationType.NON_BLOCKING, this::executeProcess)); } private void process(Fields fields, Throwable x) @@ -268,4 +271,42 @@ private void executeProcess(Fields fields, Throwable x) Response.writeError(getRequest(), getResponse(), getCallback(), x); } } + + protected static class UntilMultipartDelayedProcess extends DelayedProcess + { + private final String _contentType; + private final MultiPartConfig _config; + + public UntilMultipartDelayedProcess(Handler handler, Request request, Response response, Callback callback, String contentType, MultiPartConfig config) + { + super(handler, request, response, callback); + _contentType = contentType; + _config = config; + } + + @Override + protected void delay() + { + Request request = getRequest(); + MultiPartFormData.onParts(request, request, _contentType, _config, this::process, Invocable.from(InvocationType.NON_BLOCKING, this::executeProcess)); + } + + private void process(MultiPartFormData.Parts fields, Throwable x) + { + if (x == null) + super.process(); + else + Response.writeError(getRequest(), getResponse(), getCallback(), x); + } + + private void executeProcess(MultiPartFormData.Parts fields, Throwable x) + { + if (x == null) + // We must execute here as even though we have consumed all the input, we are probably + // invoked in a demand runnable that is serialized with any write callbacks that might be done in process + getRequest().getContext().execute(super::process); + else + Response.writeError(getRequest(), getResponse(), getCallback(), x); + } + } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java index 88c213a3132d..3d4c4ce68c1d 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java @@ -18,14 +18,18 @@ import java.io.PrintStream; import java.net.Socket; import java.nio.charset.StandardCharsets; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Exchanger; import java.util.concurrent.TimeUnit; import org.awaitility.Awaitility; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.MultiPartConfig; +import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.FormFields; import org.eclipse.jetty.server.Handler; @@ -33,6 +37,7 @@ import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; import org.junit.jupiter.api.AfterEach; @@ -503,4 +508,72 @@ public boolean handle(Request request, Response response, Callback callback) thr assertThat(content, containsString("x=[1, 2, 3]")); } } + + @Test + public void testDelayedMultipart() throws Exception + { + DelayedHandler delayedHandler = new DelayedHandler(); + _server.setAttribute(MultiPartConfig.class.getName(), new MultiPartConfig.Builder().build()); + _server.setHandler(delayedHandler); + delayedHandler.setHandler(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + CompletableFuture future = MultiPartFormData.get(request); + assertNotNull(future); + assertTrue(future.isDone()); + MultiPartFormData.Parts parts = future.get(); + assertNotNull(parts); + assertThat(parts.size(), equalTo(3)); + for (int i = 0; i < 3; i++) + { + assertThat(parts.get(i).getName(), equalTo("part" + i)); + assertThat(parts.get(i).getContentAsString(StandardCharsets.ISO_8859_1), + equalTo("This is the content of Part" + i)); + } + + response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain"); + response.write(true, BufferUtil.toBuffer("success"), callback); + return true; + } + }); + _server.start(); + + try (Socket socket = new Socket("localhost", _connector.getLocalPort())) + { + String requestContent = """ + --jettyBoundary123\r + Content-Disposition: form-data; name="part0"\r + \r + This is the content of Part0\r + --jettyBoundary123\r + Content-Disposition: form-data; name="part1"\r + \r + This is the content of Part1\r + --jettyBoundary123\r + Content-Disposition: form-data; name="part2"\r + \r + This is the content of Part2\r + --jettyBoundary123--\r + """; + String requestHeaders = String.format(""" + POST / HTTP/1.1\r + Host: localhost\r + Content-Type: multipart/form-data; boundary=jettyBoundary123\r + Content-Length: %s\r + \r + """, requestContent.getBytes(StandardCharsets.UTF_8).length); + OutputStream output = socket.getOutputStream(); + output.write((requestHeaders + requestContent).getBytes(StandardCharsets.UTF_8)); + output.flush(); + + HttpTester.Input input = HttpTester.from(socket.getInputStream()); + HttpTester.Response response = HttpTester.parseResponse(input); + assertNotNull(response); + assertEquals(HttpStatus.OK_200, response.getStatus()); + String content = new String(response.getContentBytes(), StandardCharsets.UTF_8); + assertThat(content, equalTo("success")); + } + } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index 232cd677c1b7..78f281b96bb2 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -95,6 +95,29 @@ interface Callable extends Invocable void call() throws Exception; } + // TODO javadoc + interface InvocableBiConsumer extends Invocable, BiConsumer + { + } + + static InvocableBiConsumer from(InvocationType invocationType, BiConsumer biConsumer) + { + return new InvocableBiConsumer() + { + @Override + public InvocationType getInvocationType() + { + return invocationType; + } + + @Override + public void accept(T t, U u) + { + biConsumer.accept(t, u); + } + }; + } + /** *

A {@link Runnable} decorated with an {@link InvocationType}.

*/ @@ -274,7 +297,9 @@ public InvocationType getInvocationType() * should be {@link InvocationType#NON_BLOCKING}, as the wake-up callbacks used will not block. * * @param The type of the result + * @deprecated This class in only used for deprecated usages of CompletableFuture */ + @Deprecated(forRemoval = true, since = "12.0.15") class InvocableCompletableFuture extends java.util.concurrent.CompletableFuture implements Invocable { private final InvocationType _invocationType; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index 3b1cf33f99c1..493988c0eb13 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -13,7 +13,7 @@ package org.eclipse.jetty.ee10.servlet; -import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; import jakarta.servlet.ServletRequest; import org.eclipse.jetty.http.HttpHeader; @@ -21,7 +21,9 @@ import org.eclipse.jetty.server.FormFields; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.handler.DelayedHandler; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.Fields; /** * Handler to eagerly and asynchronously read and parse {@link MimeTypes.Type#FORM_ENCODED} and @@ -29,10 +31,11 @@ * which can then consume them with blocking APIs but without blocking. * @see FormFields#from(Request) * @see ServletMultiPartFormData#from(ServletRequest) + * @deprecated use {@link DelayedHandler} */ -public class EagerFormHandler extends Handler.Wrapper +@Deprecated(forRemoval = true, since = "12.0.15") +public class EagerFormHandler extends DelayedHandler { - // TODO replace with DelayedDispatchHandler public EagerFormHandler() { this(null); @@ -54,39 +57,85 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons if (mimeType == null) return super.handle(request, response, callback); - CompletableFuture future = switch (mimeType) + return switch (mimeType) { - case FORM_ENCODED -> FormFields.from(request, InvocationType.BLOCKING); - case MULTIPART_FORM_DATA -> ServletMultiPartFormData.from(Request.as(request, ServletContextRequest.class).getServletApiRequest(), InvocationType.BLOCKING, contentType); - default -> null; + case FORM_ENCODED -> handleFormFields(request, contentType, response, callback); + case MULTIPART_FORM_DATA -> handleMultiPartFormData(request, contentType, response, callback); + default -> super.handle(request, response, callback); }; + } - if (future == null) - return super.handle(request, response, callback); - - if (future.isDone()) + protected boolean handleFormFields(Request request, String contentType, org.eclipse.jetty.server.Response response, Callback callback) throws Exception + { + BiConsumer onFields = (fields, error) -> { - if (!super.handle(request, response, callback)) - callback.failed(new IllegalStateException("Not Handled")); - } - else + try + { + if (!super.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + catch (Throwable t) + { + callback.failed(t); + } + }; + + InvocableBiConsumer executeOnFields = new InvocableBiConsumer<>() { - future.whenComplete((result, failure) -> + @Override + public void accept(Fields fields, Throwable error) { - // The result and failure are not handled here. Rather we call the next handler - // to allow the normal processing to handle the result or failure, which will be - // provided via the attribute to ServletApiRequest#getParts() - try + request.getContext().execute(() -> { - if (!super.handle(request, response, callback)) - callback.failed(new IllegalStateException("Not Handled")); - } - catch (Throwable x) + onFields.accept(fields, error); + }); + } + + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + }; + + FormFields.onFields(request, onFields, executeOnFields); + return true; + } + + protected boolean handleMultiPartFormData(Request request, String contentType, org.eclipse.jetty.server.Response response, Callback callback) throws Exception + { + BiConsumer onParts = (fields, error) -> + { + try + { + if (!super.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + catch (Throwable t) + { + callback.failed(t); + } + }; + + InvocableBiConsumer executeOnParts = new InvocableBiConsumer<>() + { + @Override + public void accept(ServletMultiPartFormData.Parts fields, Throwable error) + { + request.getContext().execute(() -> { - callback.failed(x); - } - }); - } + onParts.accept(fields, error); + }); + } + + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + }; + + ServletMultiPartFormData.onParts(Request.as(request, ServletContextRequest.class).getServletApiRequest(), contentType, onParts, executeOnParts); return true; } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 4d7083accfc7..c13f403f077a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -33,7 +33,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import jakarta.servlet.AsyncContext; @@ -634,9 +634,7 @@ public Collection getParts() throws IOException, ServletException { try { - CompletableFuture futureServletMultiPartFormData = ServletMultiPartFormData.from(this); - - _parts = futureServletMultiPartFormData.get(); + _parts = ServletMultiPartFormData.getParts(this); Collection parts = _parts.getParts(); @@ -998,10 +996,9 @@ private void extractContentParameters() throws BadMessageException ServletContextHandler contextHandler = getServletRequestInfo().getServletContextHandler(); int maxKeys = contextHandler.getMaxFormKeys(); int maxContentSize = contextHandler.getMaxFormContentSize(); - _contentParameters = FormFields.from(getRequest(), maxKeys, maxContentSize).get(); + _contentParameters = FormFields.getFields(getRequest(), maxKeys, maxContentSize); } - catch (IllegalStateException | IllegalArgumentException | ExecutionException | - InterruptedException e) + catch (IllegalStateException | IllegalArgumentException | CompletionException e) { LOG.warn(e.toString()); throw new BadMessageException("Unable to parse form content", e); @@ -1037,10 +1034,9 @@ else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && { try { - _contentParameters = FormFields.get(getRequest()).get(); + _contentParameters = FormFields.getFields(getRequest()); } - catch (IllegalStateException | IllegalArgumentException | ExecutionException | - InterruptedException e) + catch (IllegalStateException | IllegalArgumentException | CompletionException e) { LOG.warn(e.toString()); throw new BadMessageException("Unable to parse form content", e); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 34935490bf23..57b82c093bf1 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -22,6 +22,8 @@ import java.util.Collection; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.function.BiConsumer; import java.util.function.Function; import jakarta.servlet.MultipartConfigElement; @@ -53,12 +55,58 @@ */ public class ServletMultiPartFormData { + /** + * Get {@link ServletMultiPartFormData.Parts} from a servlet request, blocking if necessary. + * @param servletRequest A servlet request + */ + static Parts getParts(ServletRequest servletRequest) + { + CompletableFuture futureParts = from(servletRequest, Invocable.InvocationType.NON_BLOCKING); + return futureParts.join(); + } + + /** + * Actions to take when {@link ServletMultiPartFormData.Parts} are completely read. + * @param servletRequest A servlet request + * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. + * @param immediate The action to take if the Parts are available immediately (from within the scope of the call to this method). + * @param future The action to take when the Parts are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} + * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + */ + static void onParts(ServletRequest servletRequest, String contentType, BiConsumer immediate, Invocable.InvocableBiConsumer future) + { + CompletableFuture futureParts = from(servletRequest, future.getInvocationType(), contentType); + if (futureParts.isDone()) + { + Parts parts = null; + Throwable error = null; + try + { + parts = futureParts.get(); + } + catch (ExecutionException e) + { + error = e.getCause(); + } + catch (Throwable t) + { + error = t; + } + immediate.accept(parts, error); + } + else + { + futureParts.whenComplete(future); + } + } + /** * Get future {@link ServletMultiPartFormData.Parts} from a servlet request. * @param servletRequest A servlet request * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. * @see #from(ServletRequest, String) */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(ServletRequest servletRequest) { return from(servletRequest, Invocable.InvocationType.NON_BLOCKING, servletRequest.getContentType()); @@ -71,7 +119,8 @@ public static CompletableFuture from(ServletRequest servletRequest) * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. * @see #from(ServletRequest, String) */ - public static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType) + @Deprecated(forRemoval = true, since = "12.0.15") + static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType) { return from(servletRequest, invocationType, servletRequest.getContentType()); } @@ -82,6 +131,7 @@ public static CompletableFuture from(ServletRequest servletRequest, Invoc * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. */ + @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(ServletRequest servletRequest, String contentType) { return from(servletRequest, Invocable.InvocationType.NON_BLOCKING, contentType); @@ -94,7 +144,8 @@ public static CompletableFuture from(ServletRequest servletRequest, Strin * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. * @return A future {@link ServletMultiPartFormData.Parts}, which may have already been created and/or completed. */ - public static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType, String contentType) + @Deprecated(forRemoval = true, since = "12.0.15") + static CompletableFuture from(ServletRequest servletRequest, Invocable.InvocationType invocationType, String contentType) { // Look for an existing future (we use the future here rather than the parts as it can remember any failure). @SuppressWarnings("unchecked") From d9a54de8828a17e53197cfa8fb3546a34d9258c9 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 23 Oct 2024 10:43:47 +1100 Subject: [PATCH 22/27] Deprecated the CF APIs and replaced with explicit getXxx onXxx methods --- .../org/eclipse/jetty/util/thread/Invocable.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index 78f281b96bb2..53776d3c13c1 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -95,11 +95,24 @@ interface Callable extends Invocable void call() throws Exception; } - // TODO javadoc + /** + * An {@link Invocable} {@link BiConsumer} that provides the + * {@link InvocationType} of calls to {@link BiConsumer#accept(Object, Object)}. + * @param The first argument + * @param The second argument + */ interface InvocableBiConsumer extends Invocable, BiConsumer { } + /** + * Create an {@link InvocableBiConsumer} + * @param invocationType The {@link InvocationType} of calls to {@link BiConsumer#accept(Object, Object)} + * @param biConsumer The consumer on which to delegate calls to {@link BiConsumer#accept(Object, Object)} + * @param The first argument + * @param The second argument + * @return An {@link Invocable} {@link BiConsumer}. + */ static InvocableBiConsumer from(InvocationType invocationType, BiConsumer biConsumer) { return new InvocableBiConsumer() From f15e2982ab475a6c4ff24b70cc10df0cb7ba27eb Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 23 Oct 2024 16:24:00 +1100 Subject: [PATCH 23/27] removed more CF code --- .../eclipse/jetty/http/MultiPartFormData.java | 31 ++++++- .../authentication/FormAuthenticator.java | 2 +- .../org/eclipse/jetty/server/FormFields.java | 47 ++++++++-- .../jetty/ee10/servlet/EagerFormHandler.java | 4 +- .../servlet/ServletMultiPartFormData.java | 85 ++++++++++--------- 5 files changed, 112 insertions(+), 57 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index cbe5af83e618..bee52db65bca 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -82,11 +82,31 @@ private MultiPartFormData() } /** - * Returns {@code multipart/form-data} parts using the given {@link Content.Source} and {@link MultiPartConfig}, - * blocking if necessary. + * Get {@code multipart/form-data} {@link Parts} from an {@link Attributes}, typically + * cached there by calls to {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} + * or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} * + * @param attributes the attributes where the futureParts are cahced + * @return the parts or null + */ + public static Parts getParts(Attributes attributes) + { + Object attribute = attributes.getAttribute(MultiPartFormData.class.getName()); + if (attribute instanceof Parts parts) + return parts; + if (attribute instanceof CompletableFuture futureParts && futureParts.isDone()) + return (Parts)futureParts.join(); + return null; + } + + /** + * Get {@code multipart/form-data} {@link Parts} from a {@link Content.Source}, caching the results in an + * {@link Attributes}. If not already available, the {@code Parts} are read and parsed, blocking if necessary. + *

+ * Calls to {@code onParts} and {@code getParts} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. * @param content the source of the multipart content. - * @param attributes the attributes where the futureParts are tracked. + * @param attributes the attributes where the Parts are cached. * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. * @param config the multipart configuration. * @return the parts @@ -97,6 +117,11 @@ public static MultiPartFormData.Parts getParts(Content.Source content, Attribute } /** + * Asynchronously get {@code multipart/form-data} {@link Parts} from a {@link Content.Source}, caching the results in an + * {@link Attributes}. If not already available, the {@code Parts} are read and parsed. + *

+ * Calls to {@code onParts} and {@code getParts} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. * @param content the source of the multipart content. * @param attributes the attributes where the futureParts are tracked. * @param contentType the value of the {@link HttpHeader#CONTENT_TYPE} header. diff --git a/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java b/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java index 864fa04cc885..8c00769092eb 100644 --- a/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java +++ b/jetty-core/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/FormAuthenticator.java @@ -202,7 +202,7 @@ public Request prepareRequest(Request request, AuthenticationState authenticatio Object post = session.removeAttribute(__J_POST); if (post instanceof Fields futureFields) - FormFields.set(request, futureFields); + FormFields.setFields(request, futureFields); String method = (String)session.removeAttribute(__J_METHOD); if (method != null && request.getMethod().equals(method)) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java index d24c08a2ed97..63442b1a53a1 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java @@ -94,15 +94,24 @@ public static void set(Request request, CompletableFuture fields) * @param request The request to which to associate the fields with * @param fields A {@link CompletableFuture} that will provide either the fields or a failure. */ - public static void set(Request request, Fields fields) + public static void setFields(Request request, Fields fields) { request.setAttribute(FormFields.class.getName(), fields); } /** - * Get the Fields from a request, blocking if necessary. - * @param request The request to get the Fields from + * Get the Fields from a request. If the Fields have not been set, then attempt to parse them + * from the Request content, blocking if necessary. If the Fields have previously been read asynchronously + * by {@link #onFields(Request, BiConsumer, InvocableBiConsumer)} or similar, then those field will return + * and this method will not block. + *

+ * Calls to {@code onFields} and {@code getFields} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. + * @param request The request to get or read the Fields from * @return the Fields + * @see #onFields(Request, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #getFields(Request, int, int) */ public static Fields getFields(Request request) { @@ -111,11 +120,20 @@ public static Fields getFields(Request request) } /** - * Get the Fields from a request, blocking if necessary. - * @param request The request to get the Fields from + * Get the Fields from a request. If the Fields have not been set, then attempt to parse them + * from the Request content, blocking if necessary. If the Fields have previously been read asynchronously + * by {@link #onFields(Request, BiConsumer, InvocableBiConsumer)} or similar, then those field will return + * and this method will not block. + *

+ * Calls to {@code onFields} and {@code getFields} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. + * @param request The request to get or read the Fields from * @param maxFields The maximum number of fields to accept * @param maxLength The maximum length of fields * @return the Fields + * @see #onFields(Request, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #getFields(Request) */ public static Fields getFields(Request request, int maxFields, int maxLength) { @@ -124,11 +142,17 @@ public static Fields getFields(Request request, int maxFields, int maxLength) } /** - * Actions to take when parsing FormFields asynchronously from a request is complete - * @param request The request to parse FormFields from + * Asynchronously read and parse FormFields from a {@link Request}. + *

+ * Calls to {@code onFields} and {@code getFields} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. + * @param request The request to get or read the Fields from * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #getFields(Request) + * @see #getFields(Request, int, int) */ public static void onFields(Request request, BiConsumer immediate, InvocableBiConsumer future) { @@ -137,11 +161,17 @@ public static void onFields(Request request, BiConsumer immed /** * Actions to take when parsing FormFields asynchronously from a request is complete - * @param request The request to parse FormFields from + *

+ * Calls to {@code onFields} and {@code getFields} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. + * @param request The request to get or read the Fields from * @param charset The charset of the form. * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + * @see #onFields(Request, BiConsumer, InvocableBiConsumer) + * @see #getFields(Request) + * @see #getFields(Request, int, int) */ public static void onFields(Request request, Charset charset, BiConsumer immediate, InvocableBiConsumer future) { @@ -271,7 +301,6 @@ private static CompletableFuture from(Request request, InvocationType in return from(request, invocationType, request, charset, maxFields, maxLength); } - @Deprecated(forRemoval = true, since = "12.0.15") static CompletableFuture from(Content.Source source, InvocationType invocationType, Attributes attributes, Charset charset, int maxFields, int maxLength) { Object attr = attributes.getAttribute(FormFields.class.getName()); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index 493988c0eb13..7724b151c73d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -59,13 +59,13 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons return switch (mimeType) { - case FORM_ENCODED -> handleFormFields(request, contentType, response, callback); + case FORM_ENCODED -> handleFormFields(request, response, callback); case MULTIPART_FORM_DATA -> handleMultiPartFormData(request, contentType, response, callback); default -> super.handle(request, response, callback); }; } - protected boolean handleFormFields(Request request, String contentType, org.eclipse.jetty.server.Response response, Callback callback) throws Exception + protected boolean handleFormFields(Request request, org.eclipse.jetty.server.Response response, Callback callback) throws Exception { BiConsumer onFields = (fields, error) -> { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 57b82c093bf1..d7f695259a5a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -24,7 +24,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.function.BiConsumer; -import java.util.function.Function; import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletRequest; @@ -56,8 +55,14 @@ public class ServletMultiPartFormData { /** - * Get {@link ServletMultiPartFormData.Parts} from a servlet request, blocking if necessary. + * Get {@code multipart/form-data} {@link ServletMultiPartFormData.Parts} from a {@link ServletRequest}, caching the + * results in the request {@link ServletRequest#getAttribute(String) Attributes}. If not already available, + * the {@code Parts} are read and parsed, blocking if necessary. + *

+ * Calls to {@code onParts} and {@code getParts} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. * @param servletRequest A servlet request + * @return the parts */ static Parts getParts(ServletRequest servletRequest) { @@ -66,7 +71,12 @@ static Parts getParts(ServletRequest servletRequest) } /** - * Actions to take when {@link ServletMultiPartFormData.Parts} are completely read. + * Asynchronously get {@code multipart/form-data} {@link ServletMultiPartFormData.Parts} from a {@link ServletRequest}, + * caching the results in the request {@link ServletRequest#getAttribute(String) Attributes}. If not already available, + * the {@code Parts} are read and parsed. + *

+ * Calls to {@code onParts} and {@code getParts} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. * @param servletRequest A servlet request * @param contentType The contentType, passed as an optimization as it has likely already been retrieved. * @param immediate The action to take if the Parts are available immediately (from within the scope of the call to this method). @@ -75,6 +85,7 @@ static Parts getParts(ServletRequest servletRequest) */ static void onParts(ServletRequest servletRequest, String contentType, BiConsumer immediate, Invocable.InvocableBiConsumer future) { + // TODO inline the from method to avoid the CF CompletableFuture futureParts = from(servletRequest, future.getInvocationType(), contentType); if (futureParts.isDone()) { @@ -182,8 +193,12 @@ static CompletableFuture from(ServletRequest servletRequest, Invocable.In try { // Look for an existing future MultiPartFormData.Parts - CompletableFuture futureFormData = MultiPartFormData.get(servletContextRequest); - if (futureFormData == null) + MultiPartFormData.Parts formParts = MultiPartFormData.getParts(servletContextRequest); + if (formParts != null) + { + futureServletParts = CompletableFuture.completedFuture(new Parts(filesDirectory, formParts)); + } + else { // No existing core parts, so we need to configure the parser. ServletContextHandler contextHandler = servletContextRequest.getServletContext().getServletContextHandler(); @@ -199,9 +214,7 @@ static CompletableFuture from(ServletRequest servletRequest, Invocable.In else { int bufferSize = connection instanceof AbstractConnection c ? c.getInputBufferSize() : 2048; - InputStreamContentSource iscs = new InputStreamContentSource(servletRequest.getInputStream(), byteBufferPool); - iscs.setBufferSize(bufferSize); - source = iscs; + source = new InputStreamContentSource(servletRequest.getInputStream(), new ByteBufferPool.Sized(byteBufferPool, false, bufferSize)); } MultiPartConfig multiPartConfig = Request.getMultiPartConfig(servletContextRequest, filesDirectory) @@ -212,19 +225,31 @@ static CompletableFuture from(ServletRequest servletRequest, Invocable.In .maxSize(config.getMaxRequestSize()) .build(); - futureFormData = MultiPartFormData.from(source, invocationType, servletContextRequest, contentType, multiPartConfig); - + futureServletParts = new CompletableFuture<>(); + CompletableFuture futureConvertParts = futureServletParts; + Invocable.InvocableBiConsumer onParts = new Invocable.InvocableBiConsumer<>() + { + @Override + public void accept(MultiPartFormData.Parts parts, Throwable throwable) + { + if (throwable != null) + futureConvertParts.completeExceptionally(throwable); + else + futureConvertParts.complete(new Parts(filesDirectory, parts)); + } + + @Override + public InvocationType getInvocationType() + { + return invocationType; + } + }; + + MultiPartFormData.onParts(source, servletContextRequest, contentType, multiPartConfig, onParts, onParts); } - - // If we are already completed, ... - futureServletParts = (futureFormData.isDone()) - // we can just convert here - ? CompletableFuture.completedFuture(new Parts(filesDirectory, futureFormData.join())) - // Otherwise, when available, convert the core parts to servlet parts - : futureFormData.thenApply(new PartsFunction(invocationType, filesDirectory)); - // cache the result in attributes. servletRequest.setAttribute(ServletMultiPartFormData.class.getName(), futureServletParts); + } catch (Throwable failure) { @@ -234,30 +259,6 @@ static CompletableFuture from(ServletRequest servletRequest, Invocable.In return futureServletParts; } - private static class PartsFunction implements Function, Invocable - { - private final InvocationType _invocationType; - private final Path _filesDirectory; - - PartsFunction(InvocationType invocationType, Path filesDirectory) - { - _invocationType = invocationType; - _filesDirectory = filesDirectory; - } - - @Override - public InvocationType getInvocationType() - { - return _invocationType; - } - - @Override - public Parts apply(MultiPartFormData.Parts parts) - { - return new Parts(_filesDirectory, parts); - } - } - /** *

An ordered list of {@link Part}s that can be accessed by name.

*/ From 33b741f9e6359aa6dd7d0c1b0f731c2352b3cff0 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 23 Oct 2024 12:08:42 +0200 Subject: [PATCH 24/27] replace FutureCallback and FuturePromise usages with Blocker.* instead Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/io/Content.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java index 7dbaf9144099..d9edbedd94a6 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java @@ -43,8 +43,6 @@ import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; -import org.eclipse.jetty.util.FutureCallback; -import org.eclipse.jetty.util.FuturePromise; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.Promise; import org.slf4j.Logger; @@ -310,9 +308,11 @@ static ByteBuffer asByteBuffer(Source source) throws IOException { try { - FuturePromise promise = new FuturePromise<>(); - asByteBuffer(source, promise); - return promise.get(); + try (Blocker.Promise promise = Blocker.promise()) + { + asByteBuffer(source, promise); + return promise.block(); + } } catch (Throwable x) { @@ -483,9 +483,11 @@ static void consumeAll(Source source) throws IOException { try { - FutureCallback callback = new FutureCallback(); - consumeAll(source, callback); - callback.get(); + try (Blocker.Callback callback = Blocker.callback()) + { + consumeAll(source, callback); + callback.block(); + } } catch (Throwable x) { From d49d31211ad49a88fbda7c4630bdd077f0732edc Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 23 Oct 2024 13:24:29 +0200 Subject: [PATCH 25/27] add missing getInvocationType() returning NON_BLOCKING Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/util/FutureCallback.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java index caab6e0c4a3c..ec8bc9859e3a 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java @@ -173,6 +173,12 @@ public static void rethrow(ExecutionException e) throws IOException throw IO.rethrow(e.getCause()); } + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + @Override public String toString() { From bf7127bf03d01ab4db5b6b97d07beab209ccf1ee Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 24 Oct 2024 10:08:44 +1100 Subject: [PATCH 26/27] Converted new API on FormFields to use Promise --- .../org/eclipse/jetty/server/FormFields.java | 92 +++++++++++-------- .../jetty/server/handler/DelayedHandler.java | 34 +++---- .../eclipse/jetty/util/thread/Invocable.java | 84 +++++++++++++++++ .../jetty/ee10/servlet/EagerFormHandler.java | 45 ++++----- 4 files changed, 172 insertions(+), 83 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java index 63442b1a53a1..10f769740d9c 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/FormFields.java @@ -18,7 +18,7 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; -import java.util.function.BiConsumer; +import java.util.concurrent.ExecutionException; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MimeTypes; @@ -28,6 +28,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.CharsetStringBuilder; import org.eclipse.jetty.util.Fields; +import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.StringUtil; import static org.eclipse.jetty.util.UrlEncoded.decodeHexByte; @@ -102,27 +103,30 @@ public static void setFields(Request request, Fields fields) /** * Get the Fields from a request. If the Fields have not been set, then attempt to parse them * from the Request content, blocking if necessary. If the Fields have previously been read asynchronously - * by {@link #onFields(Request, BiConsumer, InvocableBiConsumer)} or similar, then those field will return + * by {@link #onFields(Request, Promise, InvocablePromise)} or similar, then those field will return * and this method will not block. *

* Calls to {@code onFields} and {@code getFields} methods are idempotent, and * can be called multiple times, with subsequent calls returning the results of the first call. * @param request The request to get or read the Fields from * @return the Fields - * @see #onFields(Request, BiConsumer, InvocableBiConsumer) - * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Promise, InvocablePromise) + * @see #onFields(Request, Charset, Promise, InvocablePromise) * @see #getFields(Request, int, int) */ public static Fields getFields(Request request) { - CompletableFuture fields = from(request, InvocationType.NON_BLOCKING); + int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_LENGTH_DEFAULT); + Charset charset = getFormEncodedCharset(request); + CompletableFuture fields = from(request, InvocationType.NON_BLOCKING, request, charset, maxFields, maxLength); return fields.join(); } /** * Get the Fields from a request. If the Fields have not been set, then attempt to parse them * from the Request content, blocking if necessary. If the Fields have previously been read asynchronously - * by {@link #onFields(Request, BiConsumer, InvocableBiConsumer)} or similar, then those field will return + * by {@link #onFields(Request, Promise, InvocablePromise)} or similar, then those field will return * and this method will not block. *

* Calls to {@code onFields} and {@code getFields} methods are idempotent, and @@ -131,16 +135,34 @@ public static Fields getFields(Request request) * @param maxFields The maximum number of fields to accept * @param maxLength The maximum length of fields * @return the Fields - * @see #onFields(Request, BiConsumer, InvocableBiConsumer) - * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Promise, InvocablePromise) + * @see #onFields(Request, Charset, Promise, InvocablePromise) * @see #getFields(Request) */ public static Fields getFields(Request request, int maxFields, int maxLength) { - CompletableFuture fields = from(request, InvocationType.NON_BLOCKING, getFormEncodedCharset(request), maxFields, maxLength); + Charset charset = getFormEncodedCharset(request); + CompletableFuture fields = from(request, InvocationType.NON_BLOCKING, request, charset, maxFields, maxLength); return fields.join(); } + /** + * Asynchronously read and parse FormFields from a {@link Request}. + *

+ * Calls to {@code onFields} and {@code getFields} methods are idempotent, and + * can be called multiple times, with subsequent calls returning the results of the first call. + * @param request The request to get or read the Fields from + * @param future The action to take when the FormFields are available. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} + * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. + * @see #onFields(Request, Charset, Promise, InvocablePromise) + * @see #getFields(Request) + * @see #getFields(Request, int, int) + */ + public static void onFields(Request request, InvocablePromise future) + { + onFields(request, future, future); + } + /** * Asynchronously read and parse FormFields from a {@link Request}. *

@@ -150,13 +172,17 @@ public static Fields getFields(Request request, int maxFields, int maxLength) * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. - * @see #onFields(Request, Charset, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Charset, Promise, InvocablePromise) * @see #getFields(Request) * @see #getFields(Request, int, int) */ - public static void onFields(Request request, BiConsumer immediate, InvocableBiConsumer future) + public static void onFields(Request request, Promise immediate, InvocablePromise future) { - onFields(from(request, future.getInvocationType()), immediate, future); + InvocationType invocationType = future.getInvocationType(); + int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_LENGTH_DEFAULT); + Charset charset = getFormEncodedCharset(request); + onFields(from(request, invocationType, request, charset, maxFields, maxLength), immediate, future); } /** @@ -169,16 +195,19 @@ public static void onFields(Request request, BiConsumer immed * @param immediate The action to take if the FormFields are available immediately (from within the scope of the call to this method). * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. - * @see #onFields(Request, BiConsumer, InvocableBiConsumer) + * @see #onFields(Request, Promise, InvocablePromise) * @see #getFields(Request) * @see #getFields(Request, int, int) */ - public static void onFields(Request request, Charset charset, BiConsumer immediate, InvocableBiConsumer future) + public static void onFields(Request request, Charset charset, Promise immediate, InvocablePromise future) { - onFields(from(request, future.getInvocationType(), charset), immediate, future); + InvocationType invocationType = future.getInvocationType(); + int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); + onFields(from(request, invocationType, request, charset, maxFields, maxLength), immediate, future); } - private static void onFields(CompletableFuture futureFields, BiConsumer immediate, InvocableBiConsumer future) + private static void onFields(CompletableFuture futureFields, Promise immediate, InvocablePromise future) { if (futureFields.isDone()) { @@ -188,11 +217,18 @@ private static void onFields(CompletableFuture futureFields, BiConsumer< { fields = futureFields.get(); } + catch (ExecutionException t) + { + error = t.getCause(); + } catch (Throwable t) { error = t; } - immediate.accept(fields, error); + if (error != null) + immediate.failed(error); + else + immediate.succeeded(fields); } else { @@ -232,14 +268,6 @@ public static CompletableFuture from(Request request) return from(request, maxFields, maxLength); } - @Deprecated(forRemoval = true, since = "12.0.15") - private static CompletableFuture from(Request request, InvocationType invocationType) - { - int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); - int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_LENGTH_DEFAULT); - return from(request, invocationType, getFormEncodedCharset(request), maxFields, maxLength); - } - /** * Find or create a {@link FormFields} from a {@link Content.Source}. * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, @@ -256,14 +284,6 @@ public static CompletableFuture from(Request request, Charset charset) return from(request, charset, maxFields, maxLength); } - @Deprecated(forRemoval = true, since = "12.0.15") - private static CompletableFuture from(Request request, InvocationType invocationType, Charset charset) - { - int maxFields = getContextAttribute(request.getContext(), FormFields.MAX_FIELDS_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); - int maxLength = getContextAttribute(request.getContext(), FormFields.MAX_LENGTH_ATTRIBUTE, FormFields.MAX_FIELDS_DEFAULT); - return from(request, invocationType, charset, maxFields, maxLength); - } - /** * Find or create a {@link FormFields} from a {@link Content.Source}. * @param request The {@link Request} in which to look for an existing {@link FormFields} attribute, @@ -295,12 +315,6 @@ public static CompletableFuture from(Request request, Charset charset, i return from(request, InvocationType.NON_BLOCKING, request, charset, maxFields, maxLength); } - @Deprecated(forRemoval = true, since = "12.0.15") - private static CompletableFuture from(Request request, InvocationType invocationType, Charset charset, int maxFields, int maxLength) - { - return from(request, invocationType, request, charset, maxFields, maxLength); - } - static CompletableFuture from(Content.Source source, InvocationType invocationType, Attributes attributes, Charset charset, int maxFields, int maxLength) { Object attr = attributes.getAttribute(FormFields.class.getName()); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index c5317e9dc378..5a2a6dae7dc7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -32,6 +32,7 @@ import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; +import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.Invocable; @@ -250,25 +251,24 @@ public UntilFormDelayedProcess(Handler handler, Request wrapped, Response respon @Override protected void delay() { - FormFields.onFields(getRequest(), _charset, this::process, Invocable.from(InvocationType.NON_BLOCKING, this::executeProcess)); - } + Promise onFields = new Promise<>() + { + @Override + public void failed(Throwable x) + { + Response.writeError(getRequest(), getResponse(), getCallback(), x); + } - private void process(Fields fields, Throwable x) - { - if (x == null) - super.process(); - else - Response.writeError(getRequest(), getResponse(), getCallback(), x); - } + @Override + public void succeeded(Fields result) + { + process(); + } + }; - private void executeProcess(Fields fields, Throwable x) - { - if (x == null) - // We must execute here as even though we have consumed all the input, we are probably - // invoked in a demand runnable that is serialized with any write callbacks that might be done in process - getRequest().getContext().execute(super::process); - else - Response.writeError(getRequest(), getResponse(), getCallback(), x); + InvocablePromise executeOnFields = Invocable.from(getRequest().getContext(), onFields); + + FormFields.onFields(getRequest(), _charset, onFields, executeOnFields); } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index 53776d3c13c1..b9a50b8752fe 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -16,6 +16,7 @@ import java.util.Objects; import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.BiConsumer; @@ -23,6 +24,8 @@ import java.util.function.Consumer; import java.util.function.Function; +import org.eclipse.jetty.util.Promise; + /** *

A task (typically either a {@link Runnable} or {@link Callable} * that declares how it will behave when invoked:

@@ -95,6 +98,87 @@ interface Callable extends Invocable void call() throws Exception; } + /** + * An {@link Invocable} {@link Promise} that provides the + * {@link InvocationType} of calls to {@link Promise#succeeded(Object)}. + * Also provides the {@link InvocableBiConsumer} interface as a convenient for working + * with {@link java.util.concurrent.CompletableFuture}. + * @param The result type + */ + interface InvocablePromise extends Invocable, Promise, InvocableBiConsumer + { + @Override + default void accept(R result, Throwable error) + { + if (error != null) + failed(error); + else + succeeded(result); + } + } + + /** + * Create an {@link InvocablePromise} + * @param invocationType The {@link InvocationType} of calls to the {@link InvocablePromise} + * @param promise The promise on which to delegate calls to. + * @param The type + * @return An {@link Invocable} {@link Promise}. + */ + static Promise from(InvocationType invocationType, Promise promise) + { + return new InvocablePromise() + { + @Override + public InvocationType getInvocationType() + { + return invocationType; + } + + @Override + public void succeeded(C result) + { + promise.succeeded(result); + } + + @Override + public void failed(Throwable x) + { + promise.failed(x); + } + }; + } + + /** + * Create an {@link InvocablePromise} that is {@link InvocationType#NON_BLOCKING} because + * it executes the callbacks + * @param promise The promise on which to delegate calls to. + * @param The type + * @return An {@link Invocable} {@link Promise}. + */ + static InvocablePromise from(Executor executor, Promise promise) + { + return new InvocablePromise() + { + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + + @Override + public void succeeded(C result) + { + executor.execute(() -> promise.succeeded(result)); + } + + @Override + public void failed(Throwable x) + { + executor.execute(() -> promise.failed(x)); + } + }; + } + /** * An {@link Invocable} {@link BiConsumer} that provides the * {@link InvocationType} of calls to {@link BiConsumer#accept(Object, Object)}. diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index 7724b151c73d..1f7e2a9da5c2 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -15,7 +15,6 @@ import java.util.function.BiConsumer; -import jakarta.servlet.ServletRequest; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.server.FormFields; @@ -24,16 +23,14 @@ import org.eclipse.jetty.server.handler.DelayedHandler; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; +import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.thread.Invocable; /** * Handler to eagerly and asynchronously read and parse {@link MimeTypes.Type#FORM_ENCODED} and * {@link MimeTypes.Type#MULTIPART_FORM_DATA} content prior to invoking the {@link ServletHandler}, * which can then consume them with blocking APIs but without blocking. - * @see FormFields#from(Request) - * @see ServletMultiPartFormData#from(ServletRequest) - * @deprecated use {@link DelayedHandler} */ -@Deprecated(forRemoval = true, since = "12.0.15") public class EagerFormHandler extends DelayedHandler { public EagerFormHandler() @@ -65,39 +62,33 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons }; } - protected boolean handleFormFields(Request request, org.eclipse.jetty.server.Response response, Callback callback) throws Exception + protected boolean handleFormFields(Request request, org.eclipse.jetty.server.Response response, Callback callback) { - BiConsumer onFields = (fields, error) -> - { - try - { - if (!super.handle(request, response, callback)) - callback.failed(new IllegalStateException("Not Handled")); - } - catch (Throwable t) - { - callback.failed(t); - } - }; - - InvocableBiConsumer executeOnFields = new InvocableBiConsumer<>() + Request.Handler handler = super::handle; + Promise onFields = new Promise<>() { @Override - public void accept(Fields fields, Throwable error) + public void failed(Throwable x) { - request.getContext().execute(() -> - { - onFields.accept(fields, error); - }); + callback.failed(x); } @Override - public InvocationType getInvocationType() + public void succeeded(Fields result) { - return InvocationType.NON_BLOCKING; + try + { + if (!handler.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + catch (Throwable t) + { + callback.failed(t); + } } }; + InvocablePromise executeOnFields = Invocable.from(request.getContext(), onFields); FormFields.onFields(request, onFields, executeOnFields); return true; } From 2875eeb2e7ae037b5560c9f4115d608b982782b4 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 24 Oct 2024 12:10:51 +1100 Subject: [PATCH 27/27] Converted new API on Multipart to use Promise --- .../eclipse/jetty/http/MultiPartFormData.java | 63 ++++++++++++++++--- .../jetty/server/handler/DelayedHandler.java | 34 +++++----- .../eclipse/jetty/util/thread/Invocable.java | 2 +- .../jetty/ee10/servlet/EagerFormHandler.java | 43 ++++++------- .../servlet/ServletMultiPartFormData.java | 28 +++++---- 5 files changed, 106 insertions(+), 64 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index bee52db65bca..5f2609c71c19 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -26,13 +26,14 @@ import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; -import java.util.function.BiConsumer; +import java.util.concurrent.ExecutionException; import java.util.function.Function; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.content.ContentSourceCompletableFuture; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.AutoLock; import org.eclipse.jetty.util.thread.Invocable; @@ -84,7 +85,7 @@ private MultiPartFormData() /** * Get {@code multipart/form-data} {@link Parts} from an {@link Attributes}, typically * cached there by calls to {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} - * or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} + * or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, Promise, Invocable.InvocablePromise)} * * @param attributes the attributes where the futureParts are cahced * @return the parts or null @@ -130,9 +131,10 @@ public static MultiPartFormData.Parts getParts(Content.Source content, Attribute * @param future The action to take when the FormFields are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. */ - public static void onParts(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config, BiConsumer immediate, Invocable.InvocableBiConsumer future) + public static void onParts(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config, Promise immediate, Invocable.InvocablePromise future) { CompletableFuture futureParts = from(content, future.getInvocationType(), attributes, contentType, config); + if (futureParts.isDone()) { Parts parts = null; @@ -145,7 +147,10 @@ public static void onParts(Content.Source content, Attributes attributes, String { error = t; } - immediate.accept(parts, error); + if (error != null) + immediate.failed(error); + else + immediate.succeeded(parts); } else { @@ -162,7 +167,7 @@ public static void onParts(Content.Source content, Attributes attributes, String * @param config the multipart configuration. * @return the future parts * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} - * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, Promise, Invocable.InvocablePromise)} */ @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Content.Source content, Attributes attributes, String contentType, MultiPartConfig config) @@ -179,10 +184,10 @@ public static CompletableFuture from(Content.Source con * @param config the multipart configuration. * @return the future parts * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} - * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, Promise, Invocable.InvocablePromise)} */ @Deprecated(forRemoval = true, since = "12.0.15") - public static CompletableFuture from(Content.Source content, InvocationType invocationType, Attributes attributes, String contentType, MultiPartConfig config) + private static CompletableFuture from(Content.Source content, InvocationType invocationType, Attributes attributes, String contentType, MultiPartConfig config) { // Look for an existing future (we use the future here rather than the parts as it can remember any failure). CompletableFuture futureParts = MultiPartFormData.get(attributes); @@ -211,7 +216,7 @@ public static CompletableFuture from(Content.Source con /** * Returns {@code multipart/form-data} parts using {@link MultiPartCompliance#RFC7578}. * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} - * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, Promise, Invocable.InvocablePromise)} */ @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Attributes attributes, String boundary, Function> parse) @@ -229,7 +234,7 @@ public static CompletableFuture from(Attributes attributes, String bounda * @param parse the parser completable future * @return the future parts * @deprecated use {@link #getParts(Content.Source, Attributes, String, MultiPartConfig)} - * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, BiConsumer, Invocable.InvocableBiConsumer)} + * and/or {@link #onParts(Content.Source, Attributes, String, MultiPartConfig, Promise, Invocable.InvocablePromise)} */ @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture from(Attributes attributes, MultiPartCompliance compliance, ComplianceViolation.Listener listener, String boundary, Function> parse) @@ -253,7 +258,12 @@ public static CompletableFuture from(Attributes attributes, MultiPartComp @Deprecated(forRemoval = true, since = "12.0.15") public static CompletableFuture get(Attributes attributes) { - return (CompletableFuture)attributes.getAttribute(MultiPartFormData.class.getName()); + Object value = attributes.getAttribute(MultiPartFormData.class.getName()); + if (value instanceof CompletableFuture cfp) + return (CompletableFuture)cfp; + if (value instanceof Parts parts) + return CompletableFuture.completedFuture(parts); + return null; } /** @@ -393,11 +403,44 @@ public Parser(String boundary, MultiPartCompliance multiPartCompliance, Complian parser = new MultiPart.Parser(Objects.requireNonNull(boundary), compliance, listener); } + public void parse(Content.Source content, Promise immediate, Invocable.InvocablePromise future) + { + // TODO implement without CF + CompletableFuture cf = parse(content, future.getInvocationType()); + if (cf.isDone()) + { + Parts parts = null; + Throwable failure = null; + try + { + parts = cf.get(); + } + catch (ExecutionException e) + { + failure = e.getCause(); + } + catch (Throwable t) + { + failure = t; + } + if (failure == null) + immediate.succeeded(parts); + else + immediate.failed(failure); + } + else + { + cf.whenComplete(future); + } + } + + @Deprecated(forRemoval = true, since = "12.0.15") public CompletableFuture parse(Content.Source content) { return parse(content, InvocationType.NON_BLOCKING); } + @Deprecated(forRemoval = true, since = "12.0.15") public CompletableFuture parse(Content.Source content, InvocationType invocationType) { ContentSourceCompletableFuture futureParts = new ContentSourceCompletableFuture<>(content, invocationType) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index 5a2a6dae7dc7..b6f0ebbdb54f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -288,25 +288,25 @@ public UntilMultipartDelayedProcess(Handler handler, Request request, Response r protected void delay() { Request request = getRequest(); - MultiPartFormData.onParts(request, request, _contentType, _config, this::process, Invocable.from(InvocationType.NON_BLOCKING, this::executeProcess)); - } - private void process(MultiPartFormData.Parts fields, Throwable x) - { - if (x == null) - super.process(); - else - Response.writeError(getRequest(), getResponse(), getCallback(), x); - } + Promise onParts = new Promise<>() + { + @Override + public void failed(Throwable x) + { + Response.writeError(getRequest(), getResponse(), getCallback(), x); + } - private void executeProcess(MultiPartFormData.Parts fields, Throwable x) - { - if (x == null) - // We must execute here as even though we have consumed all the input, we are probably - // invoked in a demand runnable that is serialized with any write callbacks that might be done in process - getRequest().getContext().execute(super::process); - else - Response.writeError(getRequest(), getResponse(), getCallback(), x); + @Override + public void succeeded(MultiPartFormData.Parts result) + { + process(); + } + }; + + InvocablePromise executeOnParts = Invocable.from(getRequest().getContext(), onParts); + + MultiPartFormData.onParts(request, request, _contentType, _config, onParts, executeOnParts); } } } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java index b9a50b8752fe..90729f4e5786 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/Invocable.java @@ -105,7 +105,7 @@ interface Callable extends Invocable * with {@link java.util.concurrent.CompletableFuture}. * @param The result type */ - interface InvocablePromise extends Invocable, Promise, InvocableBiConsumer + interface InvocablePromise extends Invocable, Promise, BiConsumer { @Override default void accept(R result, Throwable error) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java index 1f7e2a9da5c2..4eac62047c04 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/EagerFormHandler.java @@ -13,8 +13,6 @@ package org.eclipse.jetty.ee10.servlet; -import java.util.function.BiConsumer; - import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.server.FormFields; @@ -70,7 +68,7 @@ protected boolean handleFormFields(Request request, org.eclipse.jetty.server.Res @Override public void failed(Throwable x) { - callback.failed(x); + succeeded(null); } @Override @@ -93,39 +91,34 @@ public void succeeded(Fields result) return true; } - protected boolean handleMultiPartFormData(Request request, String contentType, org.eclipse.jetty.server.Response response, Callback callback) throws Exception + protected boolean handleMultiPartFormData(Request request, String contentType, org.eclipse.jetty.server.Response response, Callback callback) { - BiConsumer onParts = (fields, error) -> - { - try - { - if (!super.handle(request, response, callback)) - callback.failed(new IllegalStateException("Not Handled")); - } - catch (Throwable t) - { - callback.failed(t); - } - }; - - InvocableBiConsumer executeOnParts = new InvocableBiConsumer<>() + Request.Handler handler = super::handle; + Promise onParts = new Promise<>() { @Override - public void accept(ServletMultiPartFormData.Parts fields, Throwable error) + public void failed(Throwable x) { - request.getContext().execute(() -> - { - onParts.accept(fields, error); - }); + succeeded(null); } @Override - public InvocationType getInvocationType() + public void succeeded(ServletMultiPartFormData.Parts result) { - return InvocationType.NON_BLOCKING; + try + { + if (!handler.handle(request, response, callback)) + callback.failed(new IllegalStateException("Not Handled")); + } + catch (Throwable t) + { + callback.failed(t); + } } }; + InvocablePromise executeOnParts = Invocable.from(request.getContext(), onParts); + ServletMultiPartFormData.onParts(Request.as(request, ServletContextRequest.class).getServletApiRequest(), contentType, onParts, executeOnParts); return true; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index d7f695259a5a..67ff6efb50c0 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -23,7 +23,6 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; -import java.util.function.BiConsumer; import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletRequest; @@ -41,6 +40,7 @@ import org.eclipse.jetty.io.content.InputStreamContentSource; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.Invocable; @@ -83,9 +83,8 @@ static Parts getParts(ServletRequest servletRequest) * @param future The action to take when the Parts are available, if they are not available immediately. The {@link org.eclipse.jetty.util.thread.Invocable.InvocationType} * of this parameter will be used as the type for any implementation calls to {@link Content.Source#demand(Runnable)}. */ - static void onParts(ServletRequest servletRequest, String contentType, BiConsumer immediate, Invocable.InvocableBiConsumer future) + static void onParts(ServletRequest servletRequest, String contentType, Promise immediate, Invocable.InvocablePromise future) { - // TODO inline the from method to avoid the CF CompletableFuture futureParts = from(servletRequest, future.getInvocationType(), contentType); if (futureParts.isDone()) { @@ -103,7 +102,10 @@ static void onParts(ServletRequest servletRequest, String contentType, BiConsume { error = t; } - immediate.accept(parts, error); + if (error == null) + immediate.succeeded(parts); + else + immediate.failed(error); } else { @@ -225,17 +227,21 @@ static CompletableFuture from(ServletRequest servletRequest, Invocable.In .maxSize(config.getMaxRequestSize()) .build(); - futureServletParts = new CompletableFuture<>(); + futureServletParts = new Invocable.InvocableCompletableFuture<>(invocationType); CompletableFuture futureConvertParts = futureServletParts; - Invocable.InvocableBiConsumer onParts = new Invocable.InvocableBiConsumer<>() + + Invocable.InvocablePromise onParts = new Invocable.InvocablePromise<>() { @Override - public void accept(MultiPartFormData.Parts parts, Throwable throwable) + public void failed(Throwable x) + { + futureConvertParts.completeExceptionally(x); + } + + @Override + public void succeeded(MultiPartFormData.Parts parts) { - if (throwable != null) - futureConvertParts.completeExceptionally(throwable); - else - futureConvertParts.complete(new Parts(filesDirectory, parts)); + futureConvertParts.complete(new Parts(filesDirectory, parts)); } @Override