From 5a28c7484a6f4503b32ad7c67864c8e3fa085499 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 6 Jan 2021 11:34:30 +0100 Subject: [PATCH 1/4] Avoid that tests wait indefinitely. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/ValidatingConnectionPoolTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ValidatingConnectionPoolTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ValidatingConnectionPoolTest.java index 21b5e9b4bfe..e4663e93e41 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ValidatingConnectionPoolTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ValidatingConnectionPoolTest.java @@ -60,12 +60,14 @@ public class ValidatingConnectionPoolTest extends AbstractHttpClientServerTest ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scenario.getScheme()) + .timeout(5, TimeUnit.SECONDS) .send(); assertEquals(200, response.getStatus()); // The second request should be sent after the validating timeout. response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scenario.getScheme()) + .timeout(5, TimeUnit.SECONDS) .send(); assertEquals(200, response.getStatus()); } @@ -100,6 +102,7 @@ public class ValidatingConnectionPoolTest extends AbstractHttpClientServerTest ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) .scheme(scenario.getScheme()) .path("/redirect") + .timeout(5, TimeUnit.SECONDS) .send(); assertEquals(200, response.getStatus()); } From b45c32616cf42dd444e8fc0d8b6dfa0cdf01ac69 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 7 Jan 2021 12:51:06 +0100 Subject: [PATCH 2/4] Fixes #5844 - --download flag to jetty-start causes NullPointerException. Added test case, null guard and a couple small fixes. Signed-off-by: Simone Bordet --- .../administration/startup/start-jar.adoc | 4 +-- .../org/eclipse/jetty/start/StartArgs.java | 4 +-- .../org/eclipse/jetty/start/usage.txt | 2 +- .../org/eclipse/jetty/start/MainTest.java | 33 ++++++++----------- .../tests/distribution/DistributionTests.java | 25 ++++++++++++++ 5 files changed, 44 insertions(+), 24 deletions(-) diff --git a/jetty-documentation/src/main/asciidoc/administration/startup/start-jar.adoc b/jetty-documentation/src/main/asciidoc/administration/startup/start-jar.adoc index 03b51c7554b..668cd15b089 100644 --- a/jetty-documentation/src/main/asciidoc/administration/startup/start-jar.adoc +++ b/jetty-documentation/src/main/asciidoc/administration/startup/start-jar.adoc @@ -282,7 +282,7 @@ This allows for some complex hierarchies of configuration details. --download=|:: If the file does not exist at the given location, download it from the given http URI. Note: location is always relative to `${jetty.base}`. -You might need to escape the slash "\|" to use this on some environments. +You might need to escape the pipe "\|" to use this on some environments. maven.repo.uri=[url]:: The url to use to download Maven dependencies. @@ -321,4 +321,4 @@ Alternatively to create an args file for java: ---- $ java -jar start.jar --dry-run=opts,path,main,args > /tmp/args $ java @/tmp/args ----- \ No newline at end of file +---- diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java index 7161387a288..51b1a298917 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java @@ -245,9 +245,9 @@ public class StartArgs private void addFile(Module module, String uriLocation) { - if (module.isSkipFilesValidation()) + if (module != null && module.isSkipFilesValidation()) { - StartLog.debug("Not validating %s [files] for %s", module, uriLocation); + StartLog.debug("Not validating module %s [files] for %s", module, uriLocation); return; } diff --git a/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt b/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt index 530eecc55ea..bd28e85feb0 100644 --- a/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt +++ b/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt @@ -184,7 +184,7 @@ Advanced Commands: Advanced usage, If the file does not exist at the given location, download it from the given http URI. Notes: location is always relative to ${jetty.base}. - you might need to escape the slash "\|" to use + you might need to escape the pipe "\|" to use this on some environments. maven.repo.uri=[url] The url to use to download Maven dependencies. diff --git a/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java b/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java index 3c58faabcb0..8a041abec0d 100644 --- a/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java +++ b/jetty-start/src/test/java/org/eclipse/jetty/start/MainTest.java @@ -56,7 +56,7 @@ public class MainTest // cmdLineArgs.add("jetty.http.port=9090"); Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); BaseHome baseHome = main.getBaseHome(); // System.err.println(args); @@ -92,7 +92,7 @@ public class MainTest cmdLineArgs.add("STOP.WAIT=300"); Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); // System.err.println(args); // assertEquals(0, args.getEnabledModules().size(), "--stop should not build module tree"); @@ -113,7 +113,7 @@ public class MainTest // cmdLineArgs.add("--debug"); Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); main.listConfig(args); } @@ -131,8 +131,8 @@ public class MainTest List cmdLineArgs = new ArrayList<>(); Path homePath = MavenTestingUtils.getTestResourceDir("dist-home").toPath().toRealPath(); - cmdLineArgs.add("jetty.home=" + homePath.toString()); - cmdLineArgs.add("user.dir=" + homePath.toString()); + cmdLineArgs.add("jetty.home=" + homePath); + cmdLineArgs.add("user.dir=" + homePath); // JVM args cmdLineArgs.add("--exec"); @@ -146,13 +146,8 @@ public class MainTest assertThat("Extra Jar exists: " + extraJar, Files.exists(extraJar), is(true)); assertThat("Extra Dir exists: " + extraDir, Files.exists(extraDir), is(true)); - StringBuilder lib = new StringBuilder(); - lib.append("--lib="); - lib.append(extraJar.toString()); - lib.append(File.pathSeparator); - lib.append(extraDir.toString()); - - cmdLineArgs.add(lib.toString()); + String lib = "--lib=" + extraJar + File.pathSeparator + extraDir; + cmdLineArgs.add(lib); // Arbitrary XMLs cmdLineArgs.add("config.xml"); @@ -161,7 +156,7 @@ public class MainTest Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); BaseHome baseHome = main.getBaseHome(); assertThat("jetty.home", baseHome.getHome(), is(homePath.toString())); @@ -176,8 +171,8 @@ public class MainTest List cmdLineArgs = new ArrayList<>(); Path homePath = MavenTestingUtils.getTestResourceDir("dist-home").toPath().toRealPath(); - cmdLineArgs.add("jetty.home=" + homePath.toString()); - cmdLineArgs.add("user.dir=" + homePath.toString()); + cmdLineArgs.add("jetty.home=" + homePath); + cmdLineArgs.add("user.dir=" + homePath); // JVM args cmdLineArgs.add("--exec"); @@ -187,7 +182,7 @@ public class MainTest Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); BaseHome baseHome = main.getBaseHome(); assertThat("jetty.home", baseHome.getHome(), is(homePath.toString())); @@ -216,7 +211,7 @@ public class MainTest Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); BaseHome baseHome = main.getBaseHome(); assertThat("jetty.home", baseHome.getHome(), is(homePath.toString())); @@ -231,7 +226,7 @@ public class MainTest Path distPath = MavenTestingUtils.getTestResourceDir("dist-home").toPath().toRealPath(); Path homePath = MavenTestingUtils.getTargetTestingPath().resolve("dist home with spaces"); IO.copy(distPath.toFile(), homePath.toFile()); - homePath.resolve("lib/a library.jar").toFile().createNewFile(); + Files.createFile(homePath.resolve("lib/a library.jar")); List cmdLineArgs = new ArrayList<>(); cmdLineArgs.add("user.dir=" + homePath); @@ -239,7 +234,7 @@ public class MainTest cmdLineArgs.add("--lib=lib/a library.jar"); Main main = new Main(); - StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[cmdLineArgs.size()])); + StartArgs args = main.processCommandLine(cmdLineArgs.toArray(new String[0])); BaseHome baseHome = main.getBaseHome(); assertThat("jetty.home", baseHome.getHome(), is(homePath.toString())); diff --git a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java index d89185bda5d..277e8ad31d0 100644 --- a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java +++ b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java @@ -35,6 +35,7 @@ import org.eclipse.jetty.unixsocket.client.HttpClientTransportOverUnixSockets; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledOnJre; import org.junit.jupiter.api.condition.DisabledOnOs; @@ -406,4 +407,28 @@ public class DistributionTests extends AbstractDistributionTest } } } + + @Test + @Tag("external") + public void testDownload() throws Exception + { + Path jettyBase = Files.createTempDirectory("jetty_base"); + String jettyVersion = System.getProperty("jettyVersion"); + DistributionTester distribution = DistributionTester.Builder.newInstance() + .jettyVersion(jettyVersion) + .jettyBase(jettyBase) + .mavenLocalRepository(System.getProperty("mavenRepoPath")) + .build(); + + String outPath = "etc/maven-metadata.xml"; + String[] args1 = { + "--download=https://repo1.maven.org/maven2/org/eclipse/jetty/maven-metadata.xml|" + outPath + }; + try (DistributionTester.Run run = distribution.start(args1)) + { + assertTrue(run.awaitConsoleLogsFor("Base directory was modified", 15, TimeUnit.SECONDS)); + Path target = jettyBase.resolve(outPath); + assertTrue(Files.exists(target), "could not create " + target); + } + } } From 403d5ec318ffaa20f1f2c0b62df217cfc99ebbe0 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 7 Jan 2021 16:05:24 +0100 Subject: [PATCH 3/4] Fixes #5855 - HttpClient may not send queued requests. (#5856) Changed the AbstractConnectionPool.acquire() logic to call tryCreate() even when create=false. This is necessary when e.g. a sender thread T2 with create=true steals a connection whose creation was triggered by another sender thread T1. In the old code, T2 did not trigger the creation of a connection, possibly leaving a request queued. In the new code, T2 would call tryCreate(), possibly triggering the creation of a connection. This change re-introduces the fact that when sending e.g. 20 requests concurrently, 20+ connections may be created. However, it is better to err on creating more than creating less and leaving requests queued. Further refactoring moved field pending from Pool to AbstractConnectionPool. As a consequence, AbstractConnectionPool.tryCreate() now performs a demand/supply calculation to decide whether to create a new connection. Signed-off-by: Simone Bordet Co-authored-by: Greg Wilkins --- .../jetty/client/AbstractConnectionPool.java | 180 ++++++++++++------ .../eclipse/jetty/client/HttpDestination.java | 44 ++--- .../jetty/client/ConnectionPoolHelper.java | 5 +- .../jetty/client/ConnectionPoolTest.java | 88 ++++++++- .../jetty/client/HttpClientTLSTest.java | 4 +- .../http/HttpDestinationOverHTTPTest.java | 22 ++- .../java/org/eclipse/jetty/util/Pool.java | 58 ++++-- .../java/org/eclipse/jetty/util/PoolTest.java | 138 +++++++++----- 8 files changed, 366 insertions(+), 173 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java b/jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java index 61a725adc89..31d98ef3e18 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java @@ -20,9 +20,12 @@ package org.eclipse.jetty.client; import java.io.IOException; import java.util.ArrayDeque; +import java.util.ArrayList; import java.util.Collection; +import java.util.List; import java.util.Queue; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import org.eclipse.jetty.client.api.Connection; @@ -46,6 +49,7 @@ public abstract class AbstractConnectionPool extends ContainerLifeCycle implemen { private static final Logger LOG = Log.getLogger(AbstractConnectionPool.class); + private final AtomicInteger pending = new AtomicInteger(); private final HttpDestination destination; private final Callback requester; private final Pool pool; @@ -82,12 +86,23 @@ public abstract class AbstractConnectionPool extends ContainerLifeCycle implemen @Override public CompletableFuture preCreateConnections(int connectionCount) { - CompletableFuture[] futures = new CompletableFuture[connectionCount]; + if (LOG.isDebugEnabled()) + LOG.debug("Pre-creating connections {}/{}", connectionCount, getMaxConnectionCount()); + + List> futures = new ArrayList<>(); for (int i = 0; i < connectionCount; i++) { - futures[i] = tryCreateAsync(getMaxConnectionCount()); + Pool.Entry entry = pool.reserve(); + if (entry == null) + break; + pending.incrementAndGet(); + Promise.Completable future = new FutureConnection(entry); + futures.add(future); + if (LOG.isDebugEnabled()) + LOG.debug("Pre-creating connection {}/{} at {}", futures.size(), getMaxConnectionCount(), entry); + destination.newConnection(future); } - return CompletableFuture.allOf(futures); + return CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])); } protected int getMaxMultiplex() @@ -148,7 +163,7 @@ public abstract class AbstractConnectionPool extends ContainerLifeCycle implemen @ManagedAttribute(value = "The number of pending connections", readonly = true) public int getPendingConnectionCount() { - return pool.getReservedCount(); + return pending.get(); } @Override @@ -190,88 +205,82 @@ public abstract class AbstractConnectionPool extends ContainerLifeCycle implemen *

Returns an idle connection, if available; * if an idle connection is not available, and the given {@code create} parameter is {@code true} * or {@link #isMaximizeConnections()} is {@code true}, - * then schedules the opening of a new connection, if possible within the configuration of this + * then attempts to open a new connection, if possible within the configuration of this * connection pool (for example, if it does not exceed the max connection count); - * otherwise returns {@code null}.

+ * otherwise it attempts to open a new connection, if the number of queued requests is + * greater than the number of pending connections; + * if no connection is available even after the attempts to open, return {@code null}.

+ *

The {@code create} parameter is just a hint: the connection may be created even if + * {@code false}, or may not be created even if {@code true}.

* - * @param create whether to schedule the opening of a connection if no idle connections are available + * @param create a hint to attempt to open a new connection if no idle connections are available * @return an idle connection or {@code null} if no idle connections are available - * @see #tryCreate(int) + * @see #tryCreate(boolean) */ protected Connection acquire(boolean create) { if (LOG.isDebugEnabled()) LOG.debug("Acquiring create={} on {}", create, this); Connection connection = activate(); - if (connection == null && (create || isMaximizeConnections())) + if (connection == null) { - tryCreate(destination.getQueuedRequestCount()); + tryCreate(create); connection = activate(); } return connection; } /** - *

Schedules the opening of a new connection.

- *

Whether a new connection is scheduled for opening is determined by the {@code maxPending} parameter: - * if {@code maxPending} is greater than the current number of connections scheduled for opening, - * then this method returns without scheduling the opening of a new connection; - * if {@code maxPending} is negative, a new connection is always scheduled for opening.

+ *

Tries to create a new connection.

+ *

Whether a new connection is created is determined by the {@code create} parameter + * and a count of demand and supply, where the demand is derived from the number of + * queued requests, and the supply is the number of pending connections time the + * {@link #getMaxMultiplex()} factor: if the demand is less than the supply, the + * connection will not be created.

+ *

Since the number of queued requests used to derive the demand may be a stale + * value, it is possible that few more connections than strictly necessary may be + * created, but enough to satisfy the demand.

* - * @param maxPending the max desired number of connections scheduled for opening, - * or a negative number to always trigger the opening of a new connection + * @param create a hint to request to create a connection */ - protected void tryCreate(int maxPending) - { - tryCreateAsync(maxPending); - } - - private CompletableFuture tryCreateAsync(int maxPending) + protected void tryCreate(boolean create) { int connectionCount = getConnectionCount(); if (LOG.isDebugEnabled()) - LOG.debug("Try creating connection {}/{} with {}/{} pending", connectionCount, getMaxConnectionCount(), getPendingConnectionCount(), maxPending); + LOG.debug("Try creating connection {}/{} with {} pending", connectionCount, getMaxConnectionCount(), getPendingConnectionCount()); - Pool.Entry entry = pool.reserve(maxPending); + // If we have already pending sufficient multiplexed connections, then do not create another. + int multiplexed = getMaxMultiplex(); + while (true) + { + int pending = this.pending.get(); + int supply = pending * multiplexed; + int demand = destination.getQueuedRequestCount() + (create ? 1 : 0); + + boolean tryCreate = isMaximizeConnections() || supply < demand; + + if (LOG.isDebugEnabled()) + LOG.debug("Try creating({}) connection, pending/demand/supply: {}/{}/{}, result={}", create, pending, demand, supply, tryCreate); + + if (!tryCreate) + return; + + if (this.pending.compareAndSet(pending, pending + 1)) + break; + } + + // Create the connection. + Pool.Entry entry = pool.reserve(); if (entry == null) - return CompletableFuture.completedFuture(null); + { + pending.decrementAndGet(); + return; + } if (LOG.isDebugEnabled()) - LOG.debug("Creating connection {}/{}", connectionCount, getMaxConnectionCount()); - - CompletableFuture future = new CompletableFuture<>(); - destination.newConnection(new Promise() - { - @Override - public void succeeded(Connection connection) - { - if (LOG.isDebugEnabled()) - LOG.debug("Connection {}/{} creation succeeded {}", connectionCount, getMaxConnectionCount(), connection); - if (!(connection instanceof Attachable)) - { - failed(new IllegalArgumentException("Invalid connection object: " + connection)); - return; - } - ((Attachable)connection).setAttachment(entry); - onCreated(connection); - entry.enable(connection, false); - idle(connection, false); - future.complete(null); - proceed(); - } - - @Override - public void failed(Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("Connection {}/{} creation failed", connectionCount, getMaxConnectionCount(), x); - entry.remove(); - future.completeExceptionally(x); - requester.failed(x); - } - }); - - return future; + LOG.debug("Creating connection {}/{} at {}", connectionCount, getMaxConnectionCount(), entry); + Promise future = new FutureConnection(entry); + destination.newConnection(future); } protected void proceed() @@ -444,13 +453,58 @@ public abstract class AbstractConnectionPool extends ContainerLifeCycle implemen @Override public String toString() { - return String.format("%s@%x[c=%d/%d/%d,a=%d,i=%d]", + return String.format("%s@%x[c=%d/%d/%d,a=%d,i=%d,q=%d]", getClass().getSimpleName(), hashCode(), getPendingConnectionCount(), getConnectionCount(), getMaxConnectionCount(), getActiveConnectionCount(), - getIdleConnectionCount()); + getIdleConnectionCount(), + destination.getQueuedRequestCount()); + } + + private class FutureConnection extends Promise.Completable + { + private final Pool.Entry reserved; + + public FutureConnection(Pool.Entry reserved) + { + this.reserved = reserved; + } + + @Override + public void succeeded(Connection connection) + { + if (LOG.isDebugEnabled()) + LOG.debug("Connection creation succeeded {}: {}", reserved, connection); + if (connection instanceof Attachable) + { + ((Attachable)connection).setAttachment(reserved); + onCreated(connection); + pending.decrementAndGet(); + reserved.enable(connection, false); + idle(connection, false); + complete(null); + proceed(); + } + else + { + // reduce pending on failure and if not multiplexing also reduce demand + failed(new IllegalArgumentException("Invalid connection object: " + connection)); + } + } + + @Override + public void failed(Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("Connection creation failed {}", reserved, x); + // reduce pending on failure and if not multiplexing also reduce demand + pending.decrementAndGet(); + reserved.remove(); + completeExceptionally(x); + requester.failed(x); + } } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java index c8866e6dc00..42f8f8b6f1a 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java @@ -109,7 +109,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest protected void doStart() throws Exception { this.connectionPool = newConnectionPool(client); - addBean(connectionPool); + addBean(connectionPool, true); super.doStart(); Sweeper sweeper = client.getBean(Sweeper.class); if (sweeper != null && connectionPool instanceof Sweeper.Sweepable) @@ -311,9 +311,8 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest private void send(boolean create) { - if (getHttpExchanges().isEmpty()) - return; - process(create); + if (!getHttpExchanges().isEmpty()) + process(create); } private void process(boolean create) @@ -321,7 +320,10 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest // The loop is necessary in case of a new multiplexed connection, // when a single thread notified of the connection opening must // process all queued exchanges. - // In other cases looping is a work-stealing optimization. + // It is also necessary when thread T1 cannot acquire a connection + // (for example, it has been stolen by thread T2 and the pool has + // enough pending reservations). T1 returns without doing anything + // and therefore it is T2 that must send both queued requests. while (true) { Connection connection; @@ -331,14 +333,15 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest connection = connectionPool.acquire(); if (connection == null) break; - ProcessResult result = process(connection); - if (result == ProcessResult.FINISH) + boolean proceed = process(connection); + if (proceed) + create = false; + else break; - create = result == ProcessResult.RESTART; } } - private ProcessResult process(Connection connection) + private boolean process(Connection connection) { HttpClient client = getHttpClient(); HttpExchange exchange = getHttpExchanges().poll(); @@ -354,7 +357,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest LOG.debug("{} is stopping", client); connection.close(); } - return ProcessResult.FINISH; + return false; } else { @@ -372,9 +375,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest // is created. Aborting the exchange a second time will result in // a no-operation, so we just abort here to cover that edge case. exchange.abort(cause); - return getHttpExchanges().size() > 0 - ? (released ? ProcessResult.CONTINUE : ProcessResult.RESTART) - : ProcessResult.FINISH; + return getQueuedRequestCount() > 0; } SendFailure failure = send(connection, exchange); @@ -382,7 +383,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest { // Aggressively send other queued requests // in case connections are multiplexed. - return getQueuedRequestCount() > 0 ? ProcessResult.CONTINUE : ProcessResult.FINISH; + return getQueuedRequestCount() > 0; } if (LOG.isDebugEnabled()) @@ -392,10 +393,10 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest // Resend this exchange, likely on another connection, // and return false to avoid to re-enter this method. send(exchange); - return ProcessResult.FINISH; + return false; } request.abort(failure.failure); - return getHttpExchanges().size() > 0 ? ProcessResult.RESTART : ProcessResult.FINISH; + return getQueuedRequestCount() > 0; } } @@ -474,7 +475,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest // Process queued requests that may be waiting. // We may create a connection that is not // needed, but it will eventually idle timeout. - process(true); + send(true); } return removed; } @@ -541,8 +542,8 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest asString(), hashCode(), proxy == null ? "" : "(via " + proxy + ")", - exchanges.size(), - connectionPool); + getQueuedRequestCount(), + getConnectionPool()); } /** @@ -610,9 +611,4 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest } } } - - private enum ProcessResult - { - RESTART, CONTINUE, FINISH - } } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolHelper.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolHelper.java index ffe72e1ee27..ef06a6ddb8d 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolHelper.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolHelper.java @@ -19,7 +19,6 @@ package org.eclipse.jetty.client; import org.eclipse.jetty.client.api.Connection; -import org.eclipse.jetty.client.api.Destination; public class ConnectionPoolHelper { @@ -28,8 +27,8 @@ public class ConnectionPoolHelper return connectionPool.acquire(create); } - public static void tryCreate(AbstractConnectionPool connectionPool, int pending) + public static void tryCreate(AbstractConnectionPool connectionPool) { - connectionPool.tryCreate(pending); + connectionPool.tryCreate(true); } } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java index 0a679a1b155..e6afe43e15c 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ConnectionPoolTest.java @@ -23,10 +23,12 @@ import java.net.InetSocketAddress; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; import java.util.stream.IntStream; import java.util.stream.Stream; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -244,9 +246,12 @@ public class ConnectionPoolTest } @ParameterizedTest - @MethodSource("pools") + @MethodSource("poolsNoRoundRobin") public void testQueuedRequestsDontOpenTooManyConnections(ConnectionPoolFactory factory) throws Exception { + // Round robin connection pool does open a few more + // connections than expected, exclude it from this test. + startServer(new EmptyServerHandler()); HttpClientTransport transport = new HttpClientTransportOverHTTP(1); @@ -300,11 +305,10 @@ public class ConnectionPoolTest } @ParameterizedTest - @MethodSource("poolsNoRoundRobin") - public void testConcurrentRequestsDontOpenTooManyConnections(ConnectionPoolFactory factory) throws Exception + @MethodSource("pools") + public void testConcurrentRequestsWithSlowAddressResolver(ConnectionPoolFactory factory) throws Exception { - // Round robin connection pool does open a few more - // connections than expected, exclude it from this test. + // ConnectionPools may open a few more connections than expected. startServer(new EmptyServerHandler()); @@ -351,9 +355,81 @@ public class ConnectionPoolTest assertTrue(latch.await(count, TimeUnit.SECONDS)); List destinations = client.getDestinations(); assertEquals(1, destinations.size()); + } + + @ParameterizedTest + @MethodSource("pools") + public void testConcurrentRequestsAllBlockedOnServerWithLargeConnectionPool(ConnectionPoolFactory factory) throws Exception + { + int count = 50; + testConcurrentRequestsAllBlockedOnServer(factory, count, 2 * count); + } + + @ParameterizedTest + @MethodSource("pools") + public void testConcurrentRequestsAllBlockedOnServerWithExactConnectionPool(ConnectionPoolFactory factory) throws Exception + { + int count = 50; + testConcurrentRequestsAllBlockedOnServer(factory, count, count); + } + + private void testConcurrentRequestsAllBlockedOnServer(ConnectionPoolFactory factory, int count, int maxConnections) throws Exception + { + CyclicBarrier barrier = new CyclicBarrier(count); + + QueuedThreadPool serverThreads = new QueuedThreadPool(2 * count); + serverThreads.setName("server"); + server = new Server(serverThreads); + connector = new ServerConnector(server); + server.addConnector(connector); + server.setHandler(new EmptyServerHandler() + { + @Override + protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + try + { + barrier.await(); + } + catch (Exception x) + { + throw new ServletException(x); + } + } + }); + server.start(); + + QueuedThreadPool clientThreads = new QueuedThreadPool(2 * count); + clientThreads.setName("client"); + HttpClientTransport transport = new HttpClientTransportOverHTTP(1); + transport.setConnectionPoolFactory(factory.factory); + client = new HttpClient(transport, null); + client.setExecutor(clientThreads); + client.setMaxConnectionsPerDestination(maxConnections); + client.start(); + + // Send N requests to the server, all waiting on the server. + // This should open N connections, and the test verifies that + // all N are sent (i.e. the client does not keep any queued). + CountDownLatch latch = new CountDownLatch(count); + for (int i = 0; i < count; ++i) + { + int id = i; + clientThreads.execute(() -> client.newRequest("localhost", connector.getLocalPort()) + .path("/" + id) + .send(result -> + { + if (result.isSucceeded()) + latch.countDown(); + })); + } + + assertTrue(latch.await(5, TimeUnit.SECONDS), "server requests " + barrier.getNumberWaiting() + "<" + count + " - client: " + client.dump()); + List destinations = client.getDestinations(); + assertEquals(1, destinations.size()); HttpDestination destination = (HttpDestination)destinations.get(0); AbstractConnectionPool connectionPool = (AbstractConnectionPool)destination.getConnectionPool(); - assertThat(connectionPool.getConnectionCount(), Matchers.lessThanOrEqualTo(count)); + assertThat(connectionPool.getConnectionCount(), Matchers.greaterThanOrEqualTo(count)); } @ParameterizedTest diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java index 764295ef859..96e0d69232f 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java @@ -651,7 +651,7 @@ public class HttpClientTLSTest HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port); DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger the creation of a new connection, but don't use it. - ConnectionPoolHelper.tryCreate(connectionPool, -1); + ConnectionPoolHelper.tryCreate(connectionPool); // Verify that the connection has been created. while (true) { @@ -747,7 +747,7 @@ public class HttpClientTLSTest HttpDestination destination = (HttpDestination)client.getDestination(scheme, host, port); DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger the creation of a new connection, but don't use it. - ConnectionPoolHelper.tryCreate(connectionPool, -1); + ConnectionPoolHelper.tryCreate(connectionPool); // Verify that the connection has been created. while (true) { diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index edc150c5fd8..63f6907da7a 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -64,10 +64,12 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest destination.start(); DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); Connection connection = connectionPool.acquire(); - assertNull(connection); - // There are no queued requests, so no connection should be created. - connection = peekIdleConnection(connectionPool, 1, TimeUnit.SECONDS); - assertNull(connection); + if (connection == null) + { + // There are no queued requests, so the newly created connection will be idle. + connection = peekIdleConnection(connectionPool, 5, TimeUnit.SECONDS); + } + assertNotNull(connection); } } @@ -83,7 +85,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger creation of one connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); Connection connection = ConnectionPoolHelper.acquire(connectionPool, false); if (connection == null) @@ -104,7 +106,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger creation of one connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); Connection connection1 = connectionPool.acquire(); if (connection1 == null) @@ -156,7 +158,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger creation of one connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); // Make sure we entered idleCreated(). assertTrue(idleLatch.await(5, TimeUnit.SECONDS)); @@ -167,7 +169,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest assertNull(connection1); // Trigger creation of a second connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); // Second attempt also returns null because we delayed idleCreated() above. Connection connection2 = connectionPool.acquire(); @@ -195,7 +197,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger creation of one connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); Connection connection1 = connectionPool.acquire(); if (connection1 == null) @@ -232,7 +234,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest DuplexConnectionPool connectionPool = (DuplexConnectionPool)destination.getConnectionPool(); // Trigger creation of one connection. - ConnectionPoolHelper.tryCreate(connectionPool, 1); + ConnectionPoolHelper.tryCreate(connectionPool); Connection connection1 = connectionPool.acquire(); if (connection1 == null) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java index 9a9752cad47..dd705fb2fd0 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/Pool.java @@ -54,7 +54,6 @@ public class Pool implements AutoCloseable, Dumpable private final List entries = new CopyOnWriteArrayList<>(); private final int maxEntries; - private final AtomicInteger pending = new AtomicInteger(); private final StrategyType strategyType; /* @@ -137,7 +136,7 @@ public class Pool implements AutoCloseable, Dumpable public int getReservedCount() { - return pending.get(); + return (int)entries.stream().filter(Entry::isReserved).count(); } public int getIdleCount() @@ -216,7 +215,9 @@ public class Pool implements AutoCloseable, Dumpable * @return a disabled entry that is contained in the pool, * or null if the pool is closed or if the pool already contains * {@link #getMaxEntries()} entries, or the allotment has already been reserved + * @deprecated Use {@link #reserve()} instead */ + @Deprecated public Entry reserve(int allotment) { try (Locker.Lock l = locker.lock()) @@ -228,12 +229,35 @@ public class Pool implements AutoCloseable, Dumpable if (space <= 0) return null; - // The pending count is an AtomicInteger that is only ever incremented here with - // the lock held. Thus the pending count can be reduced immediately after the - // test below, but never incremented. Thus the allotment limit can be enforced. - if (allotment >= 0 && (pending.get() * getMaxMultiplex()) >= allotment) + if (allotment >= 0 && (getReservedCount() * getMaxMultiplex()) >= allotment) + return null; + + Entry entry = new Entry(); + entries.add(entry); + return entry; + } + } + + /** + * Create a new disabled slot into the pool. + * The returned entry must ultimately have the {@link Entry#enable(Object, boolean)} + * method called or be removed via {@link Pool.Entry#remove()} or + * {@link Pool#remove(Pool.Entry)}. + * + * @return a disabled entry that is contained in the pool, + * or null if the pool is closed or if the pool already contains + * {@link #getMaxEntries()} entries + */ + public Entry reserve() + { + try (Locker.Lock l = locker.lock()) + { + if (closed) + return null; + + // If we have no space + if (entries.size() >= maxEntries) return null; - pending.incrementAndGet(); Entry entry = new Entry(); entries.add(entry); @@ -342,7 +366,7 @@ public class Pool implements AutoCloseable, Dumpable if (entry != null) return entry; - entry = reserve(-1); + entry = reserve(); if (entry == null) return null; @@ -457,12 +481,11 @@ public class Pool implements AutoCloseable, Dumpable @Override public String toString() { - return String.format("%s@%x[size=%d closed=%s pending=%d]", + return String.format("%s@%x[size=%d closed=%s]", getClass().getSimpleName(), hashCode(), entries.size(), - closed, - pending.get()); + closed); } public class Entry @@ -488,7 +511,7 @@ public class Pool implements AutoCloseable, Dumpable } /** Enable a reserved entry {@link Entry}. - * An entry returned from the {@link #reserve(int)} method must be enabled with this method, + * An entry returned from the {@link #reserve()} method must be enabled with this method, * once and only once, before it is usable by the pool. * The entry may be enabled and not acquired, in which case it is immediately available to be * acquired, potentially by another thread; or it can be enabled and acquired atomically so that @@ -517,7 +540,7 @@ public class Pool implements AutoCloseable, Dumpable return false; // Pool has been closed throw new IllegalStateException("Entry already enabled: " + this); } - pending.decrementAndGet(); + return true; } @@ -618,11 +641,7 @@ public class Pool implements AutoCloseable, Dumpable boolean removed = state.compareAndSet(usageCount, -1, multiplexCount, newMultiplexCount); if (removed) - { - if (usageCount == Integer.MIN_VALUE) - pending.decrementAndGet(); return newMultiplexCount == 0; - } } } @@ -631,6 +650,11 @@ public class Pool implements AutoCloseable, Dumpable return state.getHi() < 0; } + public boolean isReserved() + { + return state.getHi() == Integer.MIN_VALUE; + } + public boolean isIdle() { long encoded = state.get(); diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/PoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/PoolTest.java index a046b395d5a..52abccdde88 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/PoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/PoolTest.java @@ -50,7 +50,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; public class PoolTest { - interface Factory { Pool getPool(int maxSize); @@ -71,7 +70,7 @@ public class PoolTest public void testAcquireRelease(Factory factory) { Pool pool = factory.getPool(1); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); assertThat(pool.size(), is(1)); assertThat(pool.getReservedCount(), is(0)); assertThat(pool.getIdleCount(), is(1)); @@ -115,7 +114,7 @@ public class PoolTest public void testRemoveBeforeRelease(Factory factory) { Pool pool = factory.getPool(1); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(pool.remove(e1), is(true)); @@ -128,7 +127,7 @@ public class PoolTest public void testCloseBeforeRelease(Factory factory) { Pool pool = factory.getPool(1); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(pool.size(), is(1)); @@ -143,15 +142,72 @@ public class PoolTest { Pool pool = factory.getPool(1); assertThat(pool.size(), is(0)); - assertThat(pool.reserve(-1), notNullValue()); + assertThat(pool.reserve(), notNullValue()); assertThat(pool.size(), is(1)); - assertThat(pool.reserve(-1), nullValue()); + assertThat(pool.reserve(), nullValue()); assertThat(pool.size(), is(1)); } @ParameterizedTest @MethodSource(value = "strategy") public void testReserve(Factory factory) + { + Pool pool = factory.getPool(2); + pool.setMaxMultiplex(2); + + // Reserve an entry + Pool.Entry e1 = pool.reserve(); + assertThat(pool.size(), is(1)); + assertThat(pool.getReservedCount(), is(1)); + assertThat(pool.getIdleCount(), is(0)); + assertThat(pool.getInUseCount(), is(0)); + + // enable the entry + e1.enable("aaa", false); + assertThat(pool.size(), is(1)); + assertThat(pool.getReservedCount(), is(0)); + assertThat(pool.getIdleCount(), is(1)); + assertThat(pool.getInUseCount(), is(0)); + + // Reserve another entry + Pool.Entry e2 = pool.reserve(); + assertThat(pool.size(), is(2)); + assertThat(pool.getReservedCount(), is(1)); + assertThat(pool.getIdleCount(), is(1)); + assertThat(pool.getInUseCount(), is(0)); + + // remove the reservation + e2.remove(); + assertThat(pool.size(), is(1)); + assertThat(pool.getReservedCount(), is(0)); + assertThat(pool.getIdleCount(), is(1)); + assertThat(pool.getInUseCount(), is(0)); + + // Reserve another entry + Pool.Entry e3 = pool.reserve(); + assertThat(pool.size(), is(2)); + assertThat(pool.getReservedCount(), is(1)); + assertThat(pool.getIdleCount(), is(1)); + assertThat(pool.getInUseCount(), is(0)); + + // enable and acquire the entry + e3.enable("bbb", true); + assertThat(pool.size(), is(2)); + assertThat(pool.getReservedCount(), is(0)); + assertThat(pool.getIdleCount(), is(1)); + assertThat(pool.getInUseCount(), is(1)); + + // can't reenable + assertThrows(IllegalStateException.class, () -> e3.enable("xxx", false)); + + // Can't enable acquired entry + Pool.Entry e = pool.acquire(); + assertThrows(IllegalStateException.class, () -> e.enable("xxx", false)); + } + + @ParameterizedTest + @MethodSource(value = "strategy") + public void testDeprecatedReserve(Factory factory) { Pool pool = factory.getPool(2); @@ -208,22 +264,8 @@ public class PoolTest assertThrows(IllegalStateException.class, () -> e3.enable("xxx", false)); // Can't enable acquired entry - assertThat(pool.acquire(), is(e1)); - assertThrows(IllegalStateException.class, () -> e1.enable("xxx", false)); - } - - @ParameterizedTest - @MethodSource(value = "strategy") - public void testReserveMaxPending(Factory factory) - { - Pool pool = factory.getPool(2); - assertThat(pool.reserve(0), nullValue()); - assertThat(pool.reserve(1), notNullValue()); - assertThat(pool.reserve(1), nullValue()); - assertThat(pool.reserve(2), notNullValue()); - assertThat(pool.reserve(2), nullValue()); - assertThat(pool.reserve(3), nullValue()); - assertThat(pool.reserve(-1), nullValue()); + Pool.Entry e = pool.acquire(); + assertThrows(IllegalStateException.class, () -> e.enable("xxx", false)); } @ParameterizedTest @@ -231,9 +273,9 @@ public class PoolTest public void testReserveNegativeMaxPending(Factory factory) { Pool pool = factory.getPool(2); - assertThat(pool.reserve(-1), notNullValue()); - assertThat(pool.reserve(-1), notNullValue()); - assertThat(pool.reserve(-1), nullValue()); + assertThat(pool.reserve(), notNullValue()); + assertThat(pool.reserve(), notNullValue()); + assertThat(pool.reserve(), nullValue()); } @ParameterizedTest @@ -241,7 +283,7 @@ public class PoolTest public void testClose(Factory factory) { Pool pool = factory.getPool(1); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); assertThat(pool.isClosed(), is(false)); pool.close(); pool.close(); @@ -249,7 +291,7 @@ public class PoolTest assertThat(pool.isClosed(), is(true)); assertThat(pool.size(), is(0)); assertThat(pool.acquire(), nullValue()); - assertThat(pool.reserve(-1), nullValue()); + assertThat(pool.reserve(), nullValue()); } @Test @@ -258,7 +300,7 @@ public class PoolTest AtomicBoolean closed = new AtomicBoolean(); Pool pool = new Pool<>(FIRST, 1); Closeable pooled = () -> closed.set(true); - pool.reserve(-1).enable(pooled, false); + pool.reserve().enable(pooled, false); assertThat(closed.get(), is(false)); pool.close(); assertThat(closed.get(), is(true)); @@ -269,7 +311,7 @@ public class PoolTest public void testRemove(Factory factory) { Pool pool = factory.getPool(1); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(pool.remove(e1), is(true)); @@ -287,8 +329,8 @@ public class PoolTest assertThat(pool.size(), is(0)); assertThat(pool.values().isEmpty(), is(true)); - pool.reserve(-1).enable("aaa", false); - pool.reserve(-1).enable("bbb", false); + pool.reserve().enable("aaa", false); + pool.reserve().enable("bbb", false); assertThat(pool.values().stream().map(Pool.Entry::getPooled).collect(toList()), equalTo(Arrays.asList("aaa", "bbb"))); assertThat(pool.size(), is(2)); } @@ -299,8 +341,8 @@ public class PoolTest { Pool pool = factory.getPool(2); - pool.reserve(-1).enable("aaa", false); - pool.reserve(-1).enable("bbb", false); + pool.reserve().enable("aaa", false); + pool.reserve().enable("bbb", false); assertThat(pool.acquire(), notNullValue()); assertThat(pool.acquire(), notNullValue()); assertThat(pool.acquire(), nullValue()); @@ -329,7 +371,7 @@ public class PoolTest { Pool pool = factory.getPool(1); pool.setMaxUsageCount(3); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(pool.release(e1), is(true)); @@ -358,8 +400,8 @@ public class PoolTest AtomicInteger b = new AtomicInteger(); counts.put("a", a); counts.put("b", b); - pool.reserve(-1).enable("a", false); - pool.reserve(-1).enable("b", false); + pool.reserve().enable("a", false); + pool.reserve().enable("b", false); counts.get(pool.acquire().getPooled()).incrementAndGet(); counts.get(pool.acquire().getPooled()).incrementAndGet(); @@ -386,7 +428,7 @@ public class PoolTest { Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(e1, notNullValue()); @@ -416,7 +458,7 @@ public class PoolTest { Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); Pool.Entry e2 = pool.acquire(); @@ -434,7 +476,7 @@ public class PoolTest { Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(pool.remove(e1), is(true)); @@ -447,7 +489,7 @@ public class PoolTest { Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); Pool.Entry e2 = pool.acquire(); @@ -471,7 +513,7 @@ public class PoolTest public void testReleaseThenRemoveNonEnabledEntry(Factory factory) { Pool pool = factory.getPool(1); - Pool.Entry e = pool.reserve(-1); + Pool.Entry e = pool.reserve(); assertThat(pool.size(), is(1)); assertThat(pool.release(e), is(false)); assertThat(pool.size(), is(1)); @@ -484,7 +526,7 @@ public class PoolTest public void testRemoveNonEnabledEntry(Factory factory) { Pool pool = factory.getPool(1); - Pool.Entry e = pool.reserve(-1); + Pool.Entry e = pool.reserve(); assertThat(pool.size(), is(1)); assertThat(pool.remove(e), is(true)); assertThat(pool.size(), is(0)); @@ -497,7 +539,7 @@ public class PoolTest Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); pool.setMaxUsageCount(3); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e0 = pool.acquire(); @@ -518,7 +560,7 @@ public class PoolTest Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); pool.setMaxUsageCount(3); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e0 = pool.acquire(); @@ -543,7 +585,7 @@ public class PoolTest Pool pool = factory.getPool(1); pool.setMaxMultiplex(2); pool.setMaxUsageCount(10); - pool.reserve(-1).enable("aaa", false); + pool.reserve().enable("aaa", false); Pool.Entry e1 = pool.acquire(); assertThat(e1.getUsageCount(), is(1)); @@ -559,7 +601,7 @@ public class PoolTest public void testDynamicMaxUsageCountChangeOverflowMaxInt(Factory factory) { Pool pool = factory.getPool(1); - Pool.Entry entry = pool.reserve(-1); + Pool.Entry entry = pool.reserve(); entry.enable("aaa", false); entry.setUsageCount(Integer.MAX_VALUE); @@ -577,9 +619,9 @@ public class PoolTest public void testDynamicMaxUsageCountChangeSweep(Factory factory) { Pool pool = factory.getPool(2); - Pool.Entry entry1 = pool.reserve(-1); + Pool.Entry entry1 = pool.reserve(); entry1.enable("aaa", false); - Pool.Entry entry2 = pool.reserve(-1); + Pool.Entry entry2 = pool.reserve(); entry2.enable("bbb", false); Pool.Entry acquired1 = pool.acquire(); From 26ef233e94889e29b2b1f1aab307c41f7f61bae6 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 11 Jan 2021 10:30:23 +0100 Subject: [PATCH 4/4] Issue #5824 Durable ConstraintMappings. (#5842) * Issue #5824 Durable ConstraintMappings. Signed-off-by: Jan Bartel --- .../security/ConstraintSecurityHandler.java | 55 +++++++++----- .../jetty/security/ConstraintTest.java | 75 ++++++++++++++++++- 2 files changed, 111 insertions(+), 19 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java index 5396e0b363f..faba57d8fcc 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/ConstraintSecurityHandler.java @@ -42,6 +42,7 @@ import org.eclipse.jetty.http.PathMap; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; +import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.URIUtil; @@ -64,6 +65,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr private static final String OMISSION_SUFFIX = ".omission"; private static final String ALL_METHODS = "*"; private final List _constraintMappings = new CopyOnWriteArrayList<>(); + private final List _durableConstraintMappings = new CopyOnWriteArrayList<>(); private final Set _roles = new CopyOnWriteArraySet<>(); private final PathMap> _constraintMap = new PathMap<>(); private boolean _denyUncoveredMethods = false; @@ -259,9 +261,6 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr return mappings; } - /** - * @return Returns the constraintMappings. - */ @Override public List getConstraintMappings() { @@ -308,8 +307,15 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr @Override public void setConstraintMappings(List constraintMappings, Set roles) { + _constraintMappings.clear(); _constraintMappings.addAll(constraintMappings); + + _durableConstraintMappings.clear(); + if (isInDurableState()) + { + _durableConstraintMappings.addAll(constraintMappings); + } if (roles == null) { @@ -331,10 +337,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr if (isStarted()) { - for (ConstraintMapping mapping : _constraintMappings) - { - processConstraintMapping(mapping); - } + _constraintMappings.stream().forEach(m -> processConstraintMapping(m)); } } @@ -358,6 +361,10 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr public void addConstraintMapping(ConstraintMapping mapping) { _constraintMappings.add(mapping); + + if (isInDurableState()) + _durableConstraintMappings.add(mapping); + if (mapping.getConstraint() != null && mapping.getConstraint().getRoles() != null) { //allow for lazy role naming: if a role is named in a security constraint, try and @@ -371,9 +378,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr } if (isStarted()) - { processConstraintMapping(mapping); - } } /** @@ -404,14 +409,7 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr @Override protected void doStart() throws Exception { - _constraintMap.clear(); - if (_constraintMappings != null) - { - for (ConstraintMapping mapping : _constraintMappings) - { - processConstraintMapping(mapping); - } - } + _constraintMappings.stream().forEach(m -> processConstraintMapping(m)); //Servlet Spec 3.1 pg 147 sec 13.8.4.2 log paths for which there are uncovered http methods checkPathsWithUncoveredHttpMethods(); @@ -424,7 +422,9 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr { super.doStop(); _constraintMap.clear(); - } + _constraintMappings.clear(); + _constraintMappings.addAll(_durableConstraintMappings); + } /** * Create and combine the constraint with the existing processed @@ -857,4 +857,23 @@ public class ConstraintSecurityHandler extends SecurityHandler implements Constr } return methods; } + + /** + * Constraints can be added to the ConstraintSecurityHandler before the + * associated context is started. These constraints should persist across + * a stop/start. Others can be added after the associated context is starting + * (eg by a web.xml/web-fragment.xml, annotation or javax.servlet api call) - + * these should not be persisted across a stop/start as they will be re-added on + * the restart. + * + * @return true if the context with which this ConstraintSecurityHandler + * has not yet started, or if there is no context, the server has not yet started. + */ + private boolean isInDurableState() + { + ContextHandler context = ContextHandler.getContextHandler(null); + Server server = getServer(); + + return (context == null && server == null) || (context != null && !context.isRunning()) || (context == null && server != null && !server.isRunning()); + } } diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 9dfd3081020..baf04618ddf 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -64,6 +64,7 @@ import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.security.Constraint; import org.eclipse.jetty.util.security.Password; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -236,6 +237,78 @@ public class ConstraintTest assertFalse(mappings.get(3).getConstraint().getAuthenticate()); } + /** + * Test that constraint mappings added before the context starts are + * retained, but those that are added after the context starts are not. + * + * @throws Exception + */ + @Test + public void testDurableConstraints() throws Exception + { + List mappings = _security.getConstraintMappings(); + assertThat("before start", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + _server.start(); + + mappings = _security.getConstraintMappings(); + assertThat("after start", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + _server.stop(); + + //After a stop, just the durable mappings are left + mappings = _security.getConstraintMappings(); + assertThat("after stop", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + _server.start(); + + //Verify the constraints are just the durables + mappings = _security.getConstraintMappings(); + assertThat("after restart", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + //Add a non-durable constraint + ConstraintMapping mapping = new ConstraintMapping(); + mapping.setPathSpec("/xxxx/*"); + Constraint constraint = new Constraint(); + constraint.setAuthenticate(false); + constraint.setName("transient"); + mapping.setConstraint(constraint); + + _security.addConstraintMapping(mapping); + + mappings = _security.getConstraintMappings(); + assertThat("after addition", getConstraintMappings().size() + 1, Matchers.equalTo(mappings.size())); + + _server.stop(); + _server.start(); + + //After a stop, only the durable mappings remain + mappings = _security.getConstraintMappings(); + assertThat("after addition", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + + //test that setConstraintMappings replaces all existing mappings whether durable or not + + //test setConstraintMappings in durable state + _server.stop(); + _security.setConstraintMappings(Collections.singletonList(mapping)); + mappings = _security.getConstraintMappings(); + assertThat("after set during stop", 1, Matchers.equalTo(mappings.size())); + _server.start(); + mappings = _security.getConstraintMappings(); + assertThat("after set after start", 1, Matchers.equalTo(mappings.size())); + + //test setConstraintMappings not in durable state + _server.stop(); + _server.start(); + assertThat("no change after start", 1, Matchers.equalTo(mappings.size())); + _security.setConstraintMappings(getConstraintMappings()); + mappings = _security.getConstraintMappings(); + assertThat("durables lost", getConstraintMappings().size(), Matchers.equalTo(mappings.size())); + _server.stop(); + mappings = _security.getConstraintMappings(); + assertThat("no mappings", 0, Matchers.equalTo(mappings.size())); + } + /** * Equivalent of Servlet Spec 3.1 pg 132, sec 13.4.1.1, Example 13-1 * @ServletSecurity @@ -655,7 +728,7 @@ public class ConstraintTest @MethodSource("basicScenarios") public void testBasic(Scenario scenario) throws Exception { - List list = new ArrayList<>(_security.getConstraintMappings()); + List list = new ArrayList<>(getConstraintMappings()); Constraint constraint6 = new Constraint(); constraint6.setAuthenticate(true);