From 6972e8d3d7ad70fcae595bf7eff4c7c78a0f625d Mon Sep 17 00:00:00 2001 From: Joshua England Date: Thu, 28 Mar 2019 12:25:27 +0000 Subject: [PATCH] Strategies added to FreePort for how to pick ports FreePort was quite inflexible in how it picked ports. If multiple threads, or processes, were concurrently trying to look for free ports in a given range there is a high likelihood of collision where they both think a port is free then race to bind to it. This change adds strategies for how to pick the ports. The LinearFreePortStrategy reflects the old strategy of linearly walking the port range and trying each port. The RandomizedFreePortStrategy is a new strategy of repeatedly trying ports in a given range in order to avoid collisions. This doesn't remove the race condition as theoretically the same situation can occur with the RandomizedFreePortStrategy as the free port is never secured. However, it does mitigate against it and reduce the likelihood of collisions over a big enough range. --- .../java/org/ops4j/net/DefaultPortTester.java | 22 ++++ .../src/main/java/org/ops4j/net/FreePort.java | 57 +++----- .../java/org/ops4j/net/FreePortStrategy.java | 13 ++ .../org/ops4j/net/LinearFreePortStrategy.java | 24 ++++ .../main/java/org/ops4j/net/PortTester.java | 14 ++ .../ops4j/net/RandomizedFreePortStrategy.java | 58 +++++++++ .../test/java/org/ops4j/net/FreePortTest.java | 27 ++-- .../net/RandomizedFreePortStrategyTest.java | 122 ++++++++++++++++++ 8 files changed, 283 insertions(+), 54 deletions(-) create mode 100644 ops4j-base-net/src/main/java/org/ops4j/net/DefaultPortTester.java create mode 100644 ops4j-base-net/src/main/java/org/ops4j/net/FreePortStrategy.java create mode 100644 ops4j-base-net/src/main/java/org/ops4j/net/LinearFreePortStrategy.java create mode 100644 ops4j-base-net/src/main/java/org/ops4j/net/PortTester.java create mode 100644 ops4j-base-net/src/main/java/org/ops4j/net/RandomizedFreePortStrategy.java create mode 100644 ops4j-base-net/src/test/java/org/ops4j/net/RandomizedFreePortStrategyTest.java diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/DefaultPortTester.java b/ops4j-base-net/src/main/java/org/ops4j/net/DefaultPortTester.java new file mode 100644 index 0000000..3e16c2d --- /dev/null +++ b/ops4j-base-net/src/main/java/org/ops4j/net/DefaultPortTester.java @@ -0,0 +1,22 @@ +package org.ops4j.net; + +import java.net.ServerSocket; + +public final class DefaultPortTester implements PortTester { + + @Override + public boolean isFree(int port) { + try + { + ServerSocket sock = new ServerSocket( port ); + sock.close(); + // is free: + return true; + // We rely on an exception thrown to determine availability or not availability. + } catch( Exception e ) + { + // not free. + return false; + } + } +} diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/FreePort.java b/ops4j-base-net/src/main/java/org/ops4j/net/FreePort.java index 5a7c021..ffb3acd 100644 --- a/ops4j-base-net/src/main/java/org/ops4j/net/FreePort.java +++ b/ops4j-base-net/src/main/java/org/ops4j/net/FreePort.java @@ -17,8 +17,6 @@ */ package org.ops4j.net; -import java.net.ServerSocket; - /** * Find a not-taken port on localhost. * First call to getPort will try to find a free port in range given by contructor. @@ -32,8 +30,9 @@ public class FreePort { - private int m_from = 0; - private int m_to = Integer.MAX_VALUE; + private final int m_from; + private final int m_to; + private final FreePortStrategy m_strategy; private int m_found = -1; /** @@ -41,11 +40,22 @@ public class FreePort * @param to End of range to search for free port (including) */ public FreePort( int from, int to ) + { + this(from, to, new LinearFreePortStrategy(new DefaultPortTester())); + } + + /** + * @param from Begin of range to search for free port (including) + * @param to End of range to search for free port (including) + */ + public FreePort( int from, int to, FreePortStrategy strategy) { m_from = from; m_to = to; + m_strategy = strategy; } + /** * Finds a free socket upon first calll and returns the same for every next call. * @@ -57,48 +67,11 @@ public int getPort() { if( m_found == -1 ) { - m_found = findFree(); + m_found = m_strategy.findFree(m_from, m_to); } return m_found; } - private int findFree() - { - for( int i = m_from; i <= m_to; i++ ) - { - if( isFree( i ) ) - { - return i; - } - } - throw new RuntimeException( "No free port in range " + m_from + ":" + m_to ); - } - - /** - * Checks a given port for availability (by creating a temporary socket) - * - * Package visibility to enable overwriting in test. - * - * @param port Port to check - * - * @return true if its free, otherwise false. - */ - boolean isFree( int port ) - { - try - { - ServerSocket sock = new ServerSocket( port ); - sock.close(); - // is free: - return true; - // We rely on an exception thrown to determine availability or not availability. - } catch( Exception e ) - { - // not free. - return false; - } - } - /** * @return Human readably String representation */ diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/FreePortStrategy.java b/ops4j-base-net/src/main/java/org/ops4j/net/FreePortStrategy.java new file mode 100644 index 0000000..b6dd3d9 --- /dev/null +++ b/ops4j-base-net/src/main/java/org/ops4j/net/FreePortStrategy.java @@ -0,0 +1,13 @@ +package org.ops4j.net; + +public interface FreePortStrategy { + + /** + * Finds a free port in the range [from, to] + * @param from + * @param to + * @return + */ + int findFree(int from, int to); + +} diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/LinearFreePortStrategy.java b/ops4j-base-net/src/main/java/org/ops4j/net/LinearFreePortStrategy.java new file mode 100644 index 0000000..35f6311 --- /dev/null +++ b/ops4j-base-net/src/main/java/org/ops4j/net/LinearFreePortStrategy.java @@ -0,0 +1,24 @@ +package org.ops4j.net; + +public final class LinearFreePortStrategy implements FreePortStrategy { + + private final PortTester m_portTester; + + public LinearFreePortStrategy(PortTester portTester) + { + this.m_portTester = portTester; + } + + public int findFree(int from, int to) + { + for(int i = from; i <= to; i++ ) + { + if( m_portTester.isFree( i ) ) + { + return i; + } + } + throw new RuntimeException( "No free port in range " + from + ":" + to); + } + +} diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/PortTester.java b/ops4j-base-net/src/main/java/org/ops4j/net/PortTester.java new file mode 100644 index 0000000..b15be30 --- /dev/null +++ b/ops4j-base-net/src/main/java/org/ops4j/net/PortTester.java @@ -0,0 +1,14 @@ +package org.ops4j.net; + +public interface PortTester { + + /** + * Checks a given port for availability + * + * @param port Port to check + * + * @return true if its free, otherwise false. + */ + boolean isFree( int port ); + +} diff --git a/ops4j-base-net/src/main/java/org/ops4j/net/RandomizedFreePortStrategy.java b/ops4j-base-net/src/main/java/org/ops4j/net/RandomizedFreePortStrategy.java new file mode 100644 index 0000000..6dcec0e --- /dev/null +++ b/ops4j-base-net/src/main/java/org/ops4j/net/RandomizedFreePortStrategy.java @@ -0,0 +1,58 @@ +package org.ops4j.net; + +import java.util.Objects; +import java.util.Random; + +public final class RandomizedFreePortStrategy implements FreePortStrategy { + + private final PortTester m_portTester; + private final int m_attempts; + private final Random m_random; + + public RandomizedFreePortStrategy(PortTester portTester) + { + this(portTester, 50); + } + + public RandomizedFreePortStrategy( PortTester portTester, int attempts) + { + this(portTester, attempts, new Random()); + } + + public RandomizedFreePortStrategy( PortTester portTester, int attempts, Random random) + { + Objects.requireNonNull(portTester); + if (attempts <= 0) { + throw new IllegalArgumentException("attempts must be greater than 0"); + } + this.m_portTester = portTester; + this.m_attempts = attempts; + this.m_random = random; + } + + public int findFree(int from, int to) + { + if (from > to) { + throw new IllegalArgumentException(); + } + int range = to - from; + if (range == 0) + { + if (m_portTester.isFree(from)) + { + return from; + } + } else + { + for (int attempt = 0; attempt < m_attempts; ++attempt) + { + int candidate = from + (m_random.nextInt(range + 1)); + if (m_portTester.isFree(candidate)) + { + return candidate; + } + } + } + throw new RuntimeException( "No free port found in range " + from + ":" + to + " after " + m_attempts + " attempts"); + } +} diff --git a/ops4j-base-net/src/test/java/org/ops4j/net/FreePortTest.java b/ops4j-base-net/src/test/java/org/ops4j/net/FreePortTest.java index 9d4f534..d6af2e7 100644 --- a/ops4j-base-net/src/test/java/org/ops4j/net/FreePortTest.java +++ b/ops4j-base-net/src/test/java/org/ops4j/net/FreePortTest.java @@ -30,11 +30,11 @@ public class FreePortTest @Test public void testFreePortFirstTimeWithOneRange() { - FreePort p = new FreePort( 1, 1 ) + PortTester portTester = new PortTester() { public boolean alreadyCalled = false; - boolean isFree( int port ) + public boolean isFree( int port ) { assertEquals( 1, port ); if( alreadyCalled ) @@ -49,18 +49,19 @@ boolean isFree( int port ) } }; - assertEquals( 1, p.getPort() ); - assertEquals( 1, p.getPort() ); + FreePort freePort = new FreePort( 1, 1, new LinearFreePortStrategy(portTester)); + assertEquals( 1, freePort.getPort() ); + assertEquals( 1, freePort.getPort() ); } @Test public void testFreePortFirstTimeWithOneRangeNonFree() { - FreePort p = new FreePort( 1, 1 ) + PortTester portTester = new PortTester() { public boolean alreadyCalled = false; - boolean isFree( int port ) + public boolean isFree( int port ) { assertEquals( 1, port ); if( alreadyCalled ) @@ -78,26 +79,27 @@ boolean isFree( int port ) } }; + FreePort freePort = new FreePort( 1, 1, new LinearFreePortStrategy(portTester)); try { - assertEquals( 1, p.getPort() ); + assertEquals( 1, freePort.getPort() ); fail( "Exception expected." ); } catch( RuntimeException e ) { } // second check should be made - assertEquals( 1, p.getPort() ); + assertEquals( 1, freePort.getPort() ); } @Test public void testRangeTraverse() { - FreePort p = new FreePort( 1, 3 ) + PortTester portTester = new PortTester() { public int alreadyCalled = 0; - boolean isFree( int port ) + public boolean isFree( int port ) { // this is the amount of required checks @@ -130,9 +132,10 @@ boolean isFree( int port ) }; - assertEquals( 3, p.getPort() ); + FreePort freePort = new FreePort( 1, 3, new LinearFreePortStrategy(portTester)); + assertEquals( 3, freePort.getPort() ); // subsequent call: - assertEquals( 3, p.getPort() ); + assertEquals( 3, freePort.getPort() ); } } diff --git a/ops4j-base-net/src/test/java/org/ops4j/net/RandomizedFreePortStrategyTest.java b/ops4j-base-net/src/test/java/org/ops4j/net/RandomizedFreePortStrategyTest.java new file mode 100644 index 0000000..c6f2674 --- /dev/null +++ b/ops4j-base-net/src/test/java/org/ops4j/net/RandomizedFreePortStrategyTest.java @@ -0,0 +1,122 @@ +package org.ops4j.net; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.Random; + +import org.junit.Test; + +public class RandomizedFreePortStrategyTest { + + public static final PortTester ALWAYS_FALSE_PORT_TESTER = new PortTester() + { + @Override + public boolean isFree(int port) + { + return false; + } + }; + + public static final PortTester ALWAYS_TRUE_PORT_TESTER = new PortTester() + { + @Override + public boolean isFree(int port) + { + return true; + } + }; + + @Test(expected = NullPointerException.class) + public void testPortTestNullGivesException() + { + new RandomizedFreePortStrategy(null, 1); + } + + @Test(expected = IllegalArgumentException.class) + public void testAttemptsZeroGivesException() + { + new RandomizedFreePortStrategy(ALWAYS_FALSE_PORT_TESTER, 0); + } + + @Test(expected = RuntimeException.class) + public void testFindFreeFailsThenExceptionThrown() + { + FreePortStrategy strategy = new RandomizedFreePortStrategy(ALWAYS_FALSE_PORT_TESTER, 1); + strategy.findFree(1, 1); + } + + @Test + public void testFindFreeWithAlwaysTruePortTester() + { + FreePortStrategy strategy = new RandomizedFreePortStrategy(ALWAYS_TRUE_PORT_TESTER, 1); + int port = strategy.findFree(1, 1); + assertEquals(port, 1); + } + + @Test(expected = IllegalArgumentException.class) + public void testFindFreeWithFromGreaterThanToGivesException() + { + FreePortStrategy strategy = new RandomizedFreePortStrategy(ALWAYS_FALSE_PORT_TESTER, 1); + strategy.findFree(2, 1); + } + + @Test + public void testRandomPortsAreTested() + { + final int from = 1; + final int to = 3; + final int range = to - from; + + final LinkedList integerSequence = new LinkedList() {{ + add(0); + add(1); + add(2); + add(0); + add(1); + add(2); + }}; + + Random random = new Random() { + @Override + public int nextInt(int bound) { + assertEquals(range + 1, bound); + assertFalse(integerSequence.isEmpty()); + return integerSequence.poll(); + } + }; + + final List testedPorts = new ArrayList<>(); + PortTester portTester = new PortTester() + { + @Override + public boolean isFree(int port) + { + testedPorts.add(port); + return false; + } + }; + + FreePortStrategy strategy = new RandomizedFreePortStrategy(portTester, integerSequence.size(), random); + boolean caught = false; + try + { + strategy.findFree(from, to); + } + catch (RuntimeException e) + { + caught = true; + } + assertTrue(caught); + for (int i = 0; i < integerSequence.size(); ++i) + { + assertEquals(integerSequence.get(i) + from, (int) testedPorts.get(i)); + } + } + +}