Jus wondering, wouldn't it be more efficient to use new ServerSocket(0).getLocalport() to find a random available port ?
On Sat, Jun 18, 2011 at 11:40 PM, <[email protected]> wrote: > Author: ngn > Date: Sat Jun 18 21:40:01 2011 > New Revision: 1137252 > > URL: http://svn.apache.org/viewvc?rev=1137252&view=rev > Log: > New implementation of passive ports, uses random assignments of ports and > has more efficient handling of wide passive port ranges. Thanks to Allen > Firstenberg for the implementation! (FTPSERVER-420, FTPSERVER-419) > > Modified: > > > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java > > > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java > > > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java > > > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java > > Modified: > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java > URL: > http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java?rev=1137252&r1=1137251&r2=1137252&view=diff > > ============================================================================== > --- > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java > (original) > +++ > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java > Sat Jun 18 21:40:01 2011 > @@ -21,6 +21,7 @@ package org.apache.ftpserver; > > import java.net.InetAddress; > import java.net.UnknownHostException; > +import java.util.Collections; > > import org.apache.ftpserver.impl.DefaultDataConnectionConfiguration; > import org.apache.ftpserver.impl.PassivePorts; > @@ -48,7 +49,7 @@ public class DataConnectionConfiguration > > private String passiveAddress; > private String passiveExternalAddress; > - private PassivePorts passivePorts = new PassivePorts(new int[] { 0 }, > true); > + private PassivePorts passivePorts = new > PassivePorts(Collections.<Integer>emptySet(), true); > private boolean implicitSsl; > > /** > > Modified: > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java > URL: > http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java?rev=1137252&r1=1137251&r2=1137252&view=diff > > ============================================================================== > --- > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java > (original) > +++ > mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java > Sat Jun 18 21:40:01 2011 > @@ -22,10 +22,15 @@ package org.apache.ftpserver.impl; > import java.io.IOException; > import java.net.ServerSocket; > import java.util.ArrayList; > -import java.util.Iterator; > +import java.util.HashSet; > import java.util.List; > +import java.util.Random; > +import java.util.Set; > import java.util.StringTokenizer; > > +import org.slf4j.Logger; > +import org.slf4j.LoggerFactory; > + > /** > * <strong>Internal class, do not use directly.</strong> > * > @@ -36,16 +41,22 @@ import java.util.StringTokenizer; > */ > public class PassivePorts { > > + private Logger log = LoggerFactory.getLogger(PassivePorts.class); > + > private static final int MAX_PORT = 65535; > > - private int[] passivePorts; > + private static final Integer MAX_PORT_INTEGER = > Integer.valueOf(MAX_PORT); > + > + private List<Integer> freeList; > > - private boolean[] reservedPorts; > + private Set<Integer> usedList; > + > + private Random r = new Random(); > > private String passivePortsString; > > private boolean checkIfBound; > - > + > /** > * Parse a string containing passive ports > * > @@ -55,27 +66,27 @@ public class PassivePorts { > * 123,124,125) or ranges of ports, including open ended > ranges > * (e.g. 123-125, 30000-, -1023). Combinations for single > ports > * and ranges is also supported. > - * @return An instance of {@link PassivePorts} based on the parsed > string > + * @return A list of Integer objects, based on the parsed string > * @throws IllegalArgumentException > * If any of of the ports in the string is invalid (e.g. > not an > * integer or too large for a port number) > */ > - private static int[] parse(final String portsString) { > - List<Integer> passivePortsList = new ArrayList<Integer>(); > + private static Set<Integer> parse(final String portsString) { > + Set<Integer> passivePortsList = new HashSet<Integer>(); > > boolean inRange = false; > - Integer lastPort = 1; > + Integer lastPort = Integer.valueOf(1); > StringTokenizer st = new StringTokenizer(portsString, ",;-", true); > while (st.hasMoreTokens()) { > String token = st.nextToken().trim(); > > if (",".equals(token) || ";".equals(token)) { > if (inRange) { > - fillRange(passivePortsList, lastPort, MAX_PORT); > + fillRange(passivePortsList, lastPort, > MAX_PORT_INTEGER); > } > > // reset state > - lastPort = 1; > + lastPort = Integer.valueOf(1); > inRange = false; > } else if ("-".equals(token)) { > inRange = true; > @@ -84,7 +95,7 @@ public class PassivePorts { > } else { > Integer port = Integer.valueOf(token); > > - verifyPort(port.intValue()); > + verifyPort(port); > > if (inRange) { > // add all numbers from last int > @@ -100,41 +111,26 @@ public class PassivePorts { > } > > if (inRange) { > - fillRange(passivePortsList, lastPort, MAX_PORT); > + fillRange(passivePortsList, lastPort, MAX_PORT_INTEGER); > } > > - int[] passivePorts = new int[passivePortsList.size()]; > - > - Iterator<Integer> iter = passivePortsList.iterator(); > - > - int counter = 0; > - while (iter.hasNext()) { > - Integer port = iter.next(); > - passivePorts[counter] = port.intValue(); > - counter++; > - } > - > - return passivePorts; > + return passivePortsList; > } > > /** > * Fill a range of ports > */ > - private static void fillRange(final List<Integer> passivePortsList, > - final Integer beginPort, final Integer endPort) { > - for (int i = beginPort.intValue(); i <= endPort.intValue(); i++) { > - addPort(passivePortsList, i); > + private static void fillRange(final Set<Integer> passivePortsList, > final Integer beginPort, final Integer endPort) { > + for (int i = beginPort; i <= endPort; i++) { > + addPort(passivePortsList, Integer.valueOf(i)); > } > } > > /** > * Add a single port if not already in list > */ > - private static void addPort(final List<Integer> passivePortsList, > - final Integer rangePort) { > - if (!passivePortsList.contains(rangePort)) { > - passivePortsList.add(rangePort); > - } > + private static void addPort(final Set<Integer> passivePortsList, final > Integer port) { > + passivePortsList.add(port); > } > > /** > @@ -142,8 +138,7 @@ public class PassivePorts { > */ > private static void verifyPort(final int port) { > if (port < 0) { > - throw new IllegalArgumentException("Port can not be negative: > " > - + port); > + throw new IllegalArgumentException("Port can not be negative: > " + port); > } else if (port > MAX_PORT) { > throw new IllegalArgumentException("Port too large: " + port); > } > @@ -155,14 +150,17 @@ public class PassivePorts { > this.passivePortsString = passivePorts; > } > > - public PassivePorts(final int[] passivePorts, boolean checkIfBound) { > - if(passivePorts == null) { > - throw new NullPointerException("passivePorts can not be > null"); > - } > - > - this.passivePorts = passivePorts.clone(); > + public PassivePorts(Set<Integer> passivePorts, boolean checkIfBound) { > + if (passivePorts == null) { > + throw new NullPointerException("passivePorts can not be > null"); > + } else if(passivePorts.isEmpty()) { > + passivePorts = new HashSet<Integer>(); > + passivePorts.add(0); > + } > + > + this.freeList = new ArrayList<Integer>(passivePorts); > + this.usedList = new HashSet<Integer>(passivePorts.size()); > > - reservedPorts = new boolean[passivePorts.length]; > this.checkIfBound = checkIfBound; > } > > @@ -171,15 +169,15 @@ public class PassivePorts { > */ > private boolean checkPortUnbound(int port) { > // is this check disabled? > - if(!checkIfBound) { > + if (!checkIfBound) { > return true; > } > - > + > // if using 0 port, it will always be available > - if(port == 0) { > + if (port == 0) { > return true; > } > - > + > ServerSocket ss = null; > try { > ss = new ServerSocket(port); > @@ -189,7 +187,7 @@ public class PassivePorts { > // port probably in use, check next > return false; > } finally { > - if(ss != null) { > + if (ss != null) { > try { > ss.close(); > } catch (IOException e) { > @@ -199,27 +197,49 @@ public class PassivePorts { > } > } > } > - > - public int reserveNextPort() { > - // search for a free port > - for (int i = 0; i < passivePorts.length; i++) { > - if (!reservedPorts[i] && checkPortUnbound(passivePorts[i])) { > - if (passivePorts[i] != 0) { > - reservedPorts[i] = true; > - } > - return passivePorts[i]; > + > + public synchronized int reserveNextPort() { > + // create a copy of the free ports, so that we can keep track of > the tested ports > + List<Integer> freeCopy = new ArrayList<Integer>(freeList); > + > + // Loop until we have found a port, or exhausted all available > ports > + while (freeCopy.size() > 0) { > + // Otherwise, pick one at random > + int i = r.nextInt(freeCopy.size()); > + Integer ret = freeCopy.get(i); > + > + if (ret == 0) { > + // "Any" port should not be removed from our free list, > + // nor added to the used list > + return 0; > + > + } else if (checkPortUnbound(ret)) { > + // Not used by someone else, so lets reserve it and return > it > + freeList.remove(i); > + usedList.add(ret); > + return ret; > + > + } else { > + freeCopy.remove(i); > + // log port unavailable, but left in pool > + log.warn("Passive port in use by another process: " + > ret); > } > } > > return -1; > } > > - public void releasePort(final int port) { > - for (int i = 0; i < passivePorts.length; i++) { > - if (passivePorts[i] == port) { > - reservedPorts[i] = false; > - break; > - } > + public synchronized void releasePort(final int port) { > + if (port == 0) { > + // Ignore port 0 being released, > + // since its not put on the used list > + > + } else if (usedList.remove(port)) { > + freeList.add(port); > + > + } else { > + // log attempt to release unused port > + log.warn("Releasing unreserved passive port: " + port); > } > } > > @@ -231,7 +251,7 @@ public class PassivePorts { > > StringBuilder sb = new StringBuilder(); > > - for (int port : passivePorts) { > + for (Integer port : freeList) { > sb.append(port); > sb.append(","); > } > > Modified: > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java > URL: > http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff > > ============================================================================== > --- > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java > (original) > +++ > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java > Sat Jun 18 21:40:01 2011 > @@ -68,10 +68,5 @@ public class PasvUsedPortTest extends Cl > > // close blocking socket > ss.close(); > - > - client.connect("localhost", getListenerPort()); > - client.login(ADMIN_USERNAME, ADMIN_PASSWORD); > - client.pasv(); > - assertEquals("227 Entering Passive Mode (127,0,0,1,48,156)", > client.getReplyString().trim()); > } > } > > Modified: > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java > URL: > http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff > > ============================================================================== > --- > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java > (original) > +++ > mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java > Sat Jun 18 21:40:01 2011 > @@ -19,11 +19,18 @@ > > package org.apache.ftpserver.impl; > > +import java.io.IOException; > +import java.net.ServerSocket; > +import java.util.ArrayList; > +import java.util.List; > + > +import junit.framework.AssertionFailedError; > import junit.framework.TestCase; > > /** > * > -* @author <a href="http://mina.apache.org";>Apache MINA Project</a>* > +* @author <a href="http://mina.apache.org";>Apache MINA Project</a> > +* > */ > public class PassivePortsTest extends TestCase { > > @@ -68,146 +75,109 @@ public class PassivePortsTest extends Te > // ok > } > } > - > - public void testParseListOfValues() { > - PassivePorts ports = new PassivePorts("123, 456,\t\n789", false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(456, ports.reserveNextPort()); > - assertEquals(789, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + > + private void assertContains( List<Integer> valid, Integer testVal ){ > + if( !valid.remove(testVal) ){ > + throw new AssertionFailedError( "Did not find "+testVal+" in > valid list "+valid ); > + } > } > + > + private void assertReserveAll(String portString, int... validPorts) { > + PassivePorts ports = new PassivePorts(portString, false); > + > + List<Integer> valid = valid(validPorts); > + > + int len = valid.size(); > + for(int i = 0; i<len; i++) { > + assertContains(valid, ports.reserveNextPort()); > + } > + assertEquals(-1, ports.reserveNextPort()); > + assertTrue(valid.isEmpty()); > > - public void testParseListOfValuesOrder() { > - PassivePorts ports = new PassivePorts("123, 789, 456", false); > + } > + > + private List<Integer> valid(int... ints) { > + List<Integer> valid = new ArrayList<Integer>(); > + for(int i : ints) { > + valid.add(i); > + } > + return valid; > + } > > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(789, ports.reserveNextPort()); > - assertEquals(456, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + public void testParseListOfValues() { > + assertReserveAll("123, 456,\t\n789", 123, 456, 789); > } > > public void testParseListOfValuesDuplicate() { > - PassivePorts ports = new PassivePorts("123, 789, 456, 789", > false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(789, ports.reserveNextPort()); > - assertEquals(456, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("123, 789, 456, 789", 123, 456, 789); > } > > public void testParseSimpleRange() { > - PassivePorts ports = new PassivePorts("123-125", false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(124, ports.reserveNextPort()); > - assertEquals(125, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("123-125", 123, 124, 125); > } > > public void testParseMultipleRanges() { > - PassivePorts ports = new PassivePorts("123-125, 127-128, 130-132", > false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(124, ports.reserveNextPort()); > - assertEquals(125, ports.reserveNextPort()); > - assertEquals(127, ports.reserveNextPort()); > - assertEquals(128, ports.reserveNextPort()); > - assertEquals(130, ports.reserveNextPort()); > - assertEquals(131, ports.reserveNextPort()); > - assertEquals(132, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("123-125, 127-128, 130-132", 123, 124, 125, 127, > 128, 130, 131, 132); > } > > public void testParseMixedRangeAndSingle() { > - PassivePorts ports = new PassivePorts("123-125, 126, 128-129", > false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(124, ports.reserveNextPort()); > - assertEquals(125, ports.reserveNextPort()); > - assertEquals(126, ports.reserveNextPort()); > - assertEquals(128, ports.reserveNextPort()); > - assertEquals(129, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("123-125, 126, 128-129", 123, 124, 125, 126, 128, > 129); > } > > public void testParseOverlapingRanges() { > - PassivePorts ports = new PassivePorts("123-125, 124-126", false); > - > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(124, ports.reserveNextPort()); > - assertEquals(125, ports.reserveNextPort()); > - assertEquals(126, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("123-125, 124-126", 123, 124, 125, 126); > } > > public void testParseOverlapingRangesorder() { > - PassivePorts ports = new PassivePorts("124-126, 123-125", false); > - > - assertEquals(124, ports.reserveNextPort()); > - assertEquals(125, ports.reserveNextPort()); > - assertEquals(126, ports.reserveNextPort()); > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("124-126, 123-125", 123, 124, 125, 126); > } > > public void testParseOpenLowerRange() { > - PassivePorts ports = new PassivePorts("9, -3", false); > - > - assertEquals(9, ports.reserveNextPort()); > - assertEquals(1, ports.reserveNextPort()); > - assertEquals(2, ports.reserveNextPort()); > - assertEquals(3, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("9, -3", 1, 2, 3, 9); > } > > public void testParseOpenUpperRange() { > - PassivePorts ports = new PassivePorts("65533-", false); > - > - assertEquals(65533, ports.reserveNextPort()); > - assertEquals(65534, ports.reserveNextPort()); > - assertEquals(65535, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("65533-", 65533, 65534, 65535); > } > > public void testParseOpenUpperRange3() { > - PassivePorts ports = new PassivePorts("65533-, 65532-", false); > - > - assertEquals(65533, ports.reserveNextPort()); > - assertEquals(65534, ports.reserveNextPort()); > - assertEquals(65535, ports.reserveNextPort()); > - assertEquals(65532, ports.reserveNextPort()); > - assertEquals(-1, ports.reserveNextPort()); > + assertReserveAll("65533-, 65532-", 65532, 65533, 65534, 65535); > } > > public void testParseOpenUpperRange2() { > - PassivePorts ports = new PassivePorts("65533-, 1", false); > + assertReserveAll("65533-, 1", 1, 65533, 65534, 65535); > + } > + > + public void testReserveNextPortBound() throws IOException { > + ServerSocket ss = new ServerSocket(0); > + > + PassivePorts ports = new > PassivePorts(Integer.toString(ss.getLocalPort()), true); > > - assertEquals(65533, ports.reserveNextPort()); > - assertEquals(65534, ports.reserveNextPort()); > - assertEquals(65535, ports.reserveNextPort()); > - assertEquals(1, ports.reserveNextPort()); > assertEquals(-1, ports.reserveNextPort()); > + > + ss.close(); > + > + assertEquals(ss.getLocalPort(), ports.reserveNextPort()); > } > > + > public void testParseRelease() { > PassivePorts ports = new PassivePorts("123, 456,789", false); > > - assertEquals(123, ports.reserveNextPort()); > - assertEquals(456, ports.reserveNextPort()); > - ports.releasePort(456); > - assertEquals(456, ports.reserveNextPort()); > + List<Integer> valid = valid(123, 456, 789); > + > + assertContains(valid, ports.reserveNextPort()); > + > + int port = ports.reserveNextPort(); > + assertContains(valid, port); > + ports.releasePort(port); > + valid.add(port); > + assertContains(valid, ports.reserveNextPort()); > + > + assertContains(valid, ports.reserveNextPort()); > > - assertEquals(789, ports.reserveNextPort()); > assertEquals(-1, ports.reserveNextPort()); > + assertEquals(0, valid.size()); > } > > - public void testNullPorts() { > - try { > - new PassivePorts((int[])null, false); > - fail("Must throw NPE"); > - } catch(NullPointerException e) { > - // ok > - } > - } > -} > +} > \ No newline at end of file > > > -- Regards, Cordialement, Emmanuel Lécharny www.iktek.com
