Skip to content

Commit

Permalink
Make tests use AvailablePortHelper, not AvailablePort (apache#5861)
Browse files Browse the repository at this point in the history
Make nearly all tests use AvailablePortHelper instead of AvailablePort
to obtain ports. See the rationale below.

Also: Remove unused methods from AvailablePort.

Rationale:

AvailablePort is inherently risky as a source of ports for tests.
Each "get available port" method obtains candidate port numbers from the
desired range by randomly sampling with replacement. This means that
multiple calls can return the same port number if the port is not put
into use between calls.

Some tests failed intermittently because they made multiple calls to
AvailablePort, received the same port on multiple calls, and unknowingly
attempted bind multiple sockets to the same port number, resulting in a
BindException. See GEODE-6622 for examples.

AvailablePortHelper does not have this problem. It obtains candidate
port numbers round robin. After returning an available port,
AvailablePortHelper will not return that port again in that JVM until it
has tested every other port in the range for availability.

To reduce the chance of different JVMs selecting each other's ports,
AvailablePortHelper selects a random starting point for its round robin
search in each JVM.

For distributed tests, DUnit further arranges for the
AvailablePortHelper in each JVM to start its round robin search in a
distinct place, maximally distant from the starting points of all other
JVMs. Because AvailablePort selects randomly from the full port range,
it cannot benefit from this techique.

The problems caused by AvailablePort are rare, but inevitable, with a
frequency determined by the total size of the port range. An upcoming
change will make the available port range much smaller (~400 ports
instead of the current ~10000 ports), which will greatly increase the
frequency of this problem. But the problem exists now, and results in
intermittent BindExceptions.
  • Loading branch information
demery-pivotal authored Dec 17, 2020
1 parent 82f5df2 commit b3867a8
Show file tree
Hide file tree
Showing 98 changed files with 278 additions and 380 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.HTTP_SERVICE_PORT;
import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
import static org.apache.geode.internal.AvailablePort.SOCKET;
import static org.apache.geode.internal.AvailablePort.getRandomAvailablePort;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.dunit.VM.getVM;
import static org.assertj.core.api.Assertions.assertThat;

Expand Down Expand Up @@ -79,7 +78,7 @@ public void setUp() throws Exception {
serverName = "server1";
locatorDir = temporaryFolder.newFolder(locatorName);
serverDir = temporaryFolder.newFolder(serverName);
httpPort = getRandomAvailablePort(SOCKET);
httpPort = getRandomAvailableTCPPort();

locatorPort = locatorVM.invoke(this::startLocator);
serverVM.invoke(this::startServer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

Expand All @@ -29,7 +30,6 @@
import org.apache.geode.cache.client.PoolManager;
import org.apache.geode.cache.server.CacheServer;
import org.apache.geode.cache.util.CacheWriterAdapter;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.test.dunit.Host;
import org.apache.geode.test.dunit.NetworkUtils;
import org.apache.geode.test.dunit.SerializableCallable;
Expand Down Expand Up @@ -65,7 +65,7 @@ public void testPoolAndLoader() throws Exception {
VM server = host.getVM(0);
VM client = host.getVM(1);

final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
server.invoke(new SerializableCallable() {
@Override
public Object call() throws IOException {
Expand Down Expand Up @@ -147,7 +147,7 @@ public void testPoolAndWriter() throws Exception {
VM server = host.getVM(0);
VM client = host.getVM(1);

final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
server.invoke(new SerializableCallable() {
@Override
public Object call() throws IOException {
Expand Down Expand Up @@ -249,7 +249,7 @@ public void testPoolLoadAndPeer() throws Exception {
VM client1 = host.getVM(1);
VM client2 = host.getVM(2);

final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
server.invoke(new SerializableCallable() {
@Override
public Object call() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.apache.geode.cache.client.internal;

import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
import static org.apache.geode.test.dunit.IgnoredException.addIgnoredException;
import static org.apache.geode.test.dunit.NetworkUtils.getServerHostName;
Expand Down Expand Up @@ -42,7 +43,6 @@
import org.apache.geode.cache.client.PoolManager;
import org.apache.geode.cache.server.CacheServer;
import org.apache.geode.distributed.internal.ServerLocationAndMemberId;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.management.membership.ClientMembership;
import org.apache.geode.management.membership.ClientMembershipEvent;
import org.apache.geode.management.membership.ClientMembershipListenerAdapter;
Expand Down Expand Up @@ -106,8 +106,7 @@ public void testNoLocators() {

try {
vm0.invoke("StartBridgeClient",
() -> startBridgeClient(null, getServerHostName(),
AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET)));
() -> startBridgeClient(null, getServerHostName(), getRandomAvailableTCPPort()));
checkLocators(vm0, new InetSocketAddress[] {}, new InetSocketAddress[] {});
putInVM(vm0);
fail("Client cache should not have been able to start");
Expand Down Expand Up @@ -250,7 +249,7 @@ public void testClientCanUseAnEmbeddedLocator() {
VM vm0 = VM.getVM(0);
VM vm1 = VM.getVM(1);

int locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int locatorPort = getRandomAvailableTCPPort();

String locators = getLocatorString(getServerHostName(), locatorPort);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static org.apache.geode.cache.Region.SEPARATOR;
import static org.apache.geode.distributed.ConfigurationProperties.ROLES;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.dunit.IgnoredException.addIgnoredException;
import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -109,7 +110,6 @@
import org.apache.geode.cache.server.ServerMetrics;
import org.apache.geode.cache.util.ObjectSizer;
import org.apache.geode.cache.util.TransactionListenerAdapter;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.internal.AvailablePortHelper;
import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.InternalInstantiator;
Expand Down Expand Up @@ -2433,7 +2433,7 @@ public void testDynamicRegionFactoryConnectionPool() throws Exception, IOExcepti
addIgnoredException("Socket Closed");
getSystem();
VM vm0 = Host.getHost(0).getVM(0);
final int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int port = getRandomAvailableTCPPort();
vm0.invoke(new SerializableCallable("Create cache server") {
@Override
public Object call() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.START_LOCATOR;
import static org.apache.geode.distributed.Locator.getLocator;
import static org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper.getDistribution;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
import static org.apache.geode.test.awaitility.GeodeAwaitility.getTimeout;
import static org.apache.geode.test.dunit.Host.getHost;
Expand Down Expand Up @@ -94,7 +95,6 @@
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
import org.apache.geode.examples.SimpleSecurityManager;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.internal.AvailablePortHelper;
import org.apache.geode.internal.cache.GemFireCacheImpl;
import org.apache.geode.internal.cache.InternalCache;
Expand Down Expand Up @@ -136,7 +136,7 @@ public ReconnectDUnitTest() {
public final void postSetUp() throws Exception {
IgnoredException.addIgnoredException("ForcedDisconnectException");
IgnoredException.addIgnoredException("Possible loss of quorum");
locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
locatorPort = getRandomAvailableTCPPort();
final int locPort = locatorPort;
Host.getHost(0).getVM(locatorVMNumber).invoke(new SerializableRunnable("start locator") {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@
import static org.apache.geode.distributed.ConfigurationProperties.TCP_PORT;
import static org.apache.geode.distributed.internal.DistributionConfig.DEFAULT_ACK_WAIT_THRESHOLD;
import static org.apache.geode.distributed.internal.OperationExecutors.SERIAL_EXECUTOR;
import static org.apache.geode.internal.AvailablePort.MULTICAST;
import static org.apache.geode.internal.AvailablePort.SOCKET;
import static org.apache.geode.internal.AvailablePort.getRandomAvailablePort;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPortRange;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableUDPPort;
import static org.apache.geode.internal.inet.LocalHostUtil.getLocalHost;
import static org.apache.geode.test.dunit.DistributedTestUtils.getDUnitLocatorPort;
import static org.apache.geode.test.dunit.LogWriterUtils.getLogWriter;
Expand Down Expand Up @@ -98,9 +97,9 @@ public class DistributedSystemDUnitTest extends JUnit4DistributedTestCase {
public void before() throws Exception {
disconnectAllFromDS();

this.mcastPort = getRandomAvailablePort(MULTICAST);
this.locatorPort = getRandomAvailablePort(SOCKET);
this.tcpPort = getRandomAvailablePort(SOCKET);
this.mcastPort = getRandomAvailableUDPPort();
this.locatorPort = getRandomAvailableTCPPort();
this.tcpPort = getRandomAvailableTCPPort();

int[] portRange = getRandomAvailableTCPPortRange(3, true);
this.lowerBoundOfPortRange = portRange[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD;
import static org.apache.geode.distributed.ConfigurationProperties.START_LOCATOR;
import static org.apache.geode.distributed.ConfigurationProperties.USE_CLUSTER_CONFIGURATION;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.internal.security.SecurableCommunicationChannel.LOCATOR;
import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
import static org.apache.geode.test.dunit.Disconnect.disconnectFromDS;
Expand Down Expand Up @@ -258,7 +259,7 @@ protected static void stopLocator() {
@Test
@Ignore("GEODE=7760 - test sometimes hangs due to product issue")
public void testCrashLocatorMultipleTimes() throws Exception {
port1 = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
port1 = getRandomAvailableTCPPort();
DistributedTestUtils.deleteLocatorStateFile(port1);
File logFile = new File("");
File stateFile = new File("locator" + port1 + "state.dat");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static org.apache.geode.distributed.ConfigurationProperties.START_LOCATOR;
import static org.apache.geode.distributed.ConfigurationProperties.USE_CLUSTER_CONFIGURATION;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.junit.Assert.assertTrue;

import java.io.BufferedReader;
Expand All @@ -35,7 +36,6 @@
import org.apache.geode.cache.client.ClientRegionShortcut;
import org.apache.geode.cache.server.CacheServer;
import org.apache.geode.distributed.Locator;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.test.dunit.Assert;
import org.apache.geode.test.dunit.Host;
import org.apache.geode.test.dunit.SerializableCallable;
Expand Down Expand Up @@ -66,7 +66,7 @@ public void testMembershipMonitoring() throws Exception {
VM vm1 = host.getVM(1);

// use a locator so we will monitor server load and record member->server mappings
int locatorPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int locatorPort = getRandomAvailableTCPPort();
Properties p = new Properties();
p.put(START_LOCATOR, "localhost[" + locatorPort + "],peer=false");
p.put(USE_CLUSTER_CONFIGURATION, "false");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/
package org.apache.geode.disttx;

import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
Expand Down Expand Up @@ -44,7 +45,6 @@
import org.apache.geode.cache.Scope;
import org.apache.geode.cache.SubscriptionAttributes;
import org.apache.geode.cache.server.CacheServer;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.internal.cache.BucketRegion;
import org.apache.geode.internal.cache.DistTXState;
import org.apache.geode.internal.cache.GemFireCacheImpl;
Expand Down Expand Up @@ -136,7 +136,7 @@ public int startServer(VM vm) {
return (Integer) vm.invoke(new SerializableCallable() {
@Override
public Object call() throws Exception {
int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int port = getRandomAvailableTCPPort();
CacheServer s = getCache().addCacheServer();
s.setPort(port);
s.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.OFF_HEAP_MEMORY_SIZE;
import static org.apache.geode.distributed.ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.dunit.Assert.assertEquals;
import static org.apache.geode.test.dunit.Assert.assertTrue;
import static org.apache.geode.test.dunit.Assert.fail;
Expand Down Expand Up @@ -48,7 +49,6 @@
import org.apache.geode.cache.server.CacheServer;
import org.apache.geode.cache30.CacheSerializableRunnable;
import org.apache.geode.cache30.ClientServerTestCase;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.internal.AvailablePortHelper;
import org.apache.geode.internal.offheap.MemoryAllocatorImpl;
import org.apache.geode.test.dunit.Assert;
Expand Down Expand Up @@ -79,7 +79,7 @@ public void testGetAllFromServer() throws Exception {
final VM server = host.getVM(0);
final VM client = host.getVM(1);
final String regionName = getUniqueName();
final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
final String serverHost = NetworkUtils.getServerHostName(server.getHost());

createBridgeServer(server, regionName, serverPort, false, false);
Expand Down Expand Up @@ -429,7 +429,7 @@ public void testGetAllWithCallbackFromServer() throws Exception {
final VM server = host.getVM(0);
final VM client = host.getVM(1);
final String regionName = getUniqueName();
final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
final String serverHost = NetworkUtils.getServerHostName(server.getHost());

createBridgeServer(server, regionName, serverPort, false, true);
Expand Down Expand Up @@ -559,7 +559,7 @@ private void testGetFromServer(final int numLocalValues) {
final VM server = host.getVM(0);
final VM client = host.getVM(1);
final String regionName = getUniqueName();
final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
final String serverHost = NetworkUtils.getServerHostName(server.getHost());

createBridgeServer(server, regionName, serverPort, false, false);
Expand Down Expand Up @@ -623,7 +623,7 @@ public void testGetAllWithExtraKeyFromServer() throws Exception {
final VM server = host.getVM(0);
final VM client = host.getVM(1);
final String regionName = getUniqueName();
final int serverPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int serverPort = getRandomAvailableTCPPort();
final String serverHost = NetworkUtils.getServerHostName(server.getHost());
final int numLocalValues = 101;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
import static org.apache.geode.distributed.ConfigurationProperties.OFF_HEAP_MEMORY_SIZE;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
import static org.apache.geode.test.dunit.LogWriterUtils.getDUnitLogLevel;
import static org.apache.geode.test.dunit.LogWriterUtils.getLogWriter;
Expand Down Expand Up @@ -100,7 +101,6 @@
import org.apache.geode.distributed.DistributedMember;
import org.apache.geode.distributed.internal.InternalDistributedSystem;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.internal.cache.execute.data.CustId;
import org.apache.geode.internal.cache.execute.data.Customer;
import org.apache.geode.internal.cache.execute.data.Order;
Expand Down Expand Up @@ -195,7 +195,7 @@ public Object call() throws Exception {
TXManagerImpl txMgr = (TXManagerImpl) getCache().getCacheTransactionManager();
txMgr.setTransactionTimeToLiveForTest(txTimeoutSecs);
if (startServer) {
int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int port = getRandomAvailableTCPPort();
CacheServer s = getCache().addCacheServer();
s.setPort(port);
s.start();
Expand All @@ -222,7 +222,7 @@ public Object call() throws Exception {
TXManagerImpl txMgr = (TXManagerImpl) cache.getCacheTransactionManager();
txMgr.setTransactionTimeToLiveForTest(10);
if (startServer) {
int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int port = getRandomAvailableTCPPort();
CacheServer s = cache.addCacheServer();
s.setPort(port);
s.start();
Expand Down Expand Up @@ -295,7 +295,7 @@ public VM getVMForTransactions(VM accessor, VM datastore) {
public Object call() throws Exception {
TXManagerImpl txMgr = (TXManagerImpl) getCache().getCacheTransactionManager();
txMgr.setTransactionTimeToLiveForTest(10);
int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int port = getRandomAvailableTCPPort();
CacheServer s = getCache().addCacheServer();
s.setPort(port);
s.start();
Expand Down Expand Up @@ -2994,7 +2994,7 @@ public void testFailoverAfterCommitDistribution() {
final int port2 = (Integer) datastore1.invoke(new SerializableCallable() {
@Override
public Object call() throws Exception {
return AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
return getRandomAvailableTCPPort();
}
});

Expand Down Expand Up @@ -3106,7 +3106,7 @@ public void testFailoverAfterCommitDistributionInvokesListenerInClientOnlyOnce()
VM client = host.getVM(3);

final int port1 = createRegionsAndStartServer(server, false);
final int port2 = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
final int port2 = getRandomAvailableTCPPort();

server.invoke(new CreateReplicateRegion("r1"));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
import static org.apache.geode.distributed.internal.membership.gms.membership.GMSJoinLeave.BYPASS_DISCOVERY_PROPERTY;
import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPort;
import static org.apache.geode.test.dunit.Assert.assertEquals;
import static org.apache.geode.test.dunit.Assert.assertFalse;
import static org.apache.geode.test.dunit.Assert.assertNotNull;
Expand Down Expand Up @@ -62,7 +63,6 @@
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.distributed.internal.membership.api.MemberDisconnectedException;
import org.apache.geode.distributed.internal.membership.api.MembershipManagerHelper;
import org.apache.geode.internal.AvailablePort;
import org.apache.geode.test.awaitility.GeodeAwaitility;
import org.apache.geode.test.dunit.Host;
import org.apache.geode.test.dunit.IgnoredException;
Expand Down Expand Up @@ -172,7 +172,7 @@ public Object call() throws Exception {
} else {
getCache().createRegionFactory(RegionShortcut.PARTITION).create(PR_REG_NAME);
}
int port = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
int port = getRandomAvailableTCPPort();
CacheServer s = getCache().addCacheServer();
s.setPort(port);
s.start();
Expand Down
Loading

0 comments on commit b3867a8

Please sign in to comment.