From 0ff1bfdd5e4d6db6b6dd78279b6317fe3bbf9c57 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 30 Nov 2020 17:01:09 +0100 Subject: [PATCH 1/2] fix bug that messes up the max usage count when the entries' max usage counter overflows Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/util/Pool.java | 10 +++++++++- .../java/org/eclipse/jetty/util/PoolTest.java | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) 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 fc0b63368af9..0cc0cb2426d3 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 @@ -449,6 +449,12 @@ public class Entry this.state = new AtomicBiInteger(Integer.MIN_VALUE, 0); } + // for testing only + void setUsageCount(int usageCount) + { + this.state.getAndSetHi(usageCount); + } + /** Enable a reserved entry {@link Entry}. * An entry returned from the {@link #reserve(int)} method must be enabled with this method, * once and only once, before it is usable by the pool. @@ -527,7 +533,9 @@ boolean tryAcquire() if (closed || multiplexingCount >= maxMultiplex || (currentMaxUsageCount > 0 && usageCount >= currentMaxUsageCount)) return false; - if (state.compareAndSet(encoded, usageCount + 1, multiplexingCount + 1)) + // Prevent overflowing the usage counter by capping it at Integer.MAX_VALUE. + int newUsageCount = usageCount == Integer.MAX_VALUE ? Integer.MAX_VALUE : usageCount + 1; + if (state.compareAndSet(encoded, newUsageCount, multiplexingCount + 1)) return true; } } 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 00071f247af4..e6f9b38a9f73 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 @@ -554,6 +554,24 @@ public void testUsageCountAfterReachingMaxMultiplexLimit(Factory factory) assertThat(e1.getUsageCount(), is(2)); } + @ParameterizedTest + @MethodSource(value = "strategy") + public void testDynamicMaxUsageCountChangeOverflowMaxInt(Factory factory) + { + Pool pool = factory.getPool(1); + Pool.Entry entry = pool.reserve(-1); + entry.enable("aaa", false); + entry.setUsageCount(Integer.MAX_VALUE); + + Pool.Entry acquired1 = pool.acquire(); + assertThat(acquired1, notNullValue()); + assertThat(pool.release(acquired1), is(true)); + + pool.setMaxUsageCount(1); + Pool.Entry acquired2 = pool.acquire(); + assertThat(acquired2, nullValue()); + } + @Test public void testConfigLimits() { From 44e6f4aeb8bdbcb140ba99ba6480c9127844758b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 1 Dec 2020 10:25:20 +0100 Subject: [PATCH 2/2] sweep the entries list when the max usage count is changed Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/util/Pool.java | 50 ++++++++++++++++--- .../java/org/eclipse/jetty/util/PoolTest.java | 18 +++++++ 2 files changed, 61 insertions(+), 7 deletions(-) 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 0cc0cb2426d3..7fa2c441c98a 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 @@ -29,6 +29,7 @@ import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import java.util.stream.Collectors; import org.eclipse.jetty.util.component.Dumpable; import org.eclipse.jetty.util.component.DumpableCollection; @@ -166,16 +167,42 @@ public final void setMaxMultiplex(int maxMultiplex) this.maxMultiplex = maxMultiplex; } + /** + * Get the maximum number of times the entries of the pool + * can be acquired. + * @return the max usage count. + */ public int getMaxUsageCount() { return maxUsageCount; } + /** + * Change the max usage count of the pool's entries. All existing + * idle entries over this new max usage are removed and closed. + * @param maxUsageCount the max usage count. + */ public final void setMaxUsageCount(int maxUsageCount) { if (maxUsageCount == 0) throw new IllegalArgumentException("Max usage count must be != 0"); this.maxUsageCount = maxUsageCount; + + // Iterate the entries, remove overused ones and collect a list of the closeable removed ones. + List copy; + try (Locker.Lock l = locker.lock()) + { + if (closed) + return; + + copy = entries.stream() + .filter(entry -> entry.isIdleAndOverUsed() && remove(entry) && entry.pooled instanceof Closeable) + .map(entry -> (Closeable)entry.pooled) + .collect(Collectors.toList()); + } + + // Iterate the copy and close the collected entries. + copy.forEach(IO::close); } /** @@ -571,13 +598,6 @@ boolean tryRelease() return !(overUsed && newMultiplexingCount == 0); } - public boolean isOverUsed() - { - int currentMaxUsageCount = maxUsageCount; - int usageCount = state.getHi(); - return currentMaxUsageCount > 0 && usageCount >= currentMaxUsageCount; - } - /** * Try to mark the entry as removed. * @return true if the entry has to be removed from the containing pool, false otherwise. @@ -618,6 +638,22 @@ public boolean isInUse() return AtomicBiInteger.getHi(encoded) >= 0 && AtomicBiInteger.getLo(encoded) > 0; } + public boolean isOverUsed() + { + int currentMaxUsageCount = maxUsageCount; + int usageCount = state.getHi(); + return currentMaxUsageCount > 0 && usageCount >= currentMaxUsageCount; + } + + boolean isIdleAndOverUsed() + { + int currentMaxUsageCount = maxUsageCount; + long encoded = state.get(); + int usageCount = AtomicBiInteger.getHi(encoded); + int multiplexCount = AtomicBiInteger.getLo(encoded); + return currentMaxUsageCount > 0 && usageCount >= currentMaxUsageCount && multiplexCount == 0; + } + public int getUsageCount() { return Math.max(state.getHi(), 0); 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 e6f9b38a9f73..a046b395d5a8 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 @@ -572,6 +572,24 @@ public void testDynamicMaxUsageCountChangeOverflowMaxInt(Factory factory) assertThat(acquired2, nullValue()); } + @ParameterizedTest + @MethodSource(value = "strategy") + public void testDynamicMaxUsageCountChangeSweep(Factory factory) + { + Pool pool = factory.getPool(2); + Pool.Entry entry1 = pool.reserve(-1); + entry1.enable("aaa", false); + Pool.Entry entry2 = pool.reserve(-1); + entry2.enable("bbb", false); + + Pool.Entry acquired1 = pool.acquire(); + assertThat(acquired1, notNullValue()); + assertThat(pool.release(acquired1), is(true)); + + pool.setMaxUsageCount(1); + assertThat(pool.size(), is(1)); + } + @Test public void testConfigLimits() {