Occasionally try to raise the limit by allowing an extra connection.

bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiter.java

@@ -3,6 +3,7 @@ package org.briarproject.bramble.plugin.bluetooth;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 import org.briarproject.bramble.api.plugin.duplex.DuplexTransportConnection;
 
+import static java.util.concurrent.TimeUnit.MINUTES;
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 @NotNullByDefault
@@ -13,6 +14,13 @@ interface BluetoothConnectionLimiter {
 	 */
 	long STABILITY_PERIOD_MS = SECONDS.toMillis(90);
 
+	/**
+	 * The minimum interval between attempts to raise the connection limit.
+	 * This is longer than {@link #STABILITY_PERIOD_MS} so we don't start
+	 * another attempt before knowing the outcome of the last one.
+	 */
+	long MIN_ATTEMPT_INTERVAL_MS = MINUTES.toMillis(5);
+
 	/**
 	 * Informs the limiter that key agreement has started.
 	 */

bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImpl.java

@@ -39,6 +39,8 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter {
 	private boolean keyAgreementInProgress = false;
 	@GuardedBy("lock")
 	private int connectionLimit = 1;
+	@GuardedBy("lock")
+	private long timeOfLastAttempt = 0;
 
 	@Inject
 	BluetoothConnectionLimiterImpl(EventBus eventBus, Clock clock) {
@@ -67,36 +69,31 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter {
 	public boolean canOpenContactConnection() {
 		synchronized (lock) {
 			if (keyAgreementInProgress) {
-				LOG.info("Can't open contact connection during key agreement");
+				LOG.info("Refusing contact connection during key agreement");
-				return false;
-			}
-			considerRaisingConnectionLimit(clock.currentTimeMillis());
-			if (connections.size() >= connectionLimit) {
-				LOG.info("Can't open contact connection due to limit");
 				return false;
 			} else {
-				LOG.info("Can open contact connection");
+				long now = clock.currentTimeMillis();
-				return true;
+				return isContactConnectionAllowedByLimit(now);
 			}
 		}
 	}
 
 	@Override
 	public boolean contactConnectionOpened(DuplexTransportConnection conn) {
-		boolean accept = true;
+		boolean accept;
 		synchronized (lock) {
 			if (keyAgreementInProgress) {
 				LOG.info("Refusing contact connection during key agreement");
 				accept = false;
 			} else {
 				long now = clock.currentTimeMillis();
-				considerRaisingConnectionLimit(now);
+				accept = isContactConnectionAllowedByLimit(now);
-				if (connections.size() > connectionLimit) {
+				if (accept) {
-					LOG.info("Refusing contact connection due to limit");
-					accept = false;
-				} else {
-					LOG.info("Accepting contact connection");
 					connections.add(new ConnectionRecord(conn, now));
+					if (connections.size() > connectionLimit) {
+						LOG.info("Attempting to raise connection limit");
+						timeOfLastAttempt = now;
+					}
 				}
 			}
 		}
@@ -146,13 +143,31 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter {
 		}
 	}
 
+	@GuardedBy("lock")
+	private boolean isContactConnectionAllowedByLimit(long now) {
+		considerRaisingConnectionLimit(now);
+		if (connections.size() > connectionLimit) {
+			LOG.info("Refusing contact connection, above limit");
+			return false;
+		} else if (connections.size() < connectionLimit) {
+			LOG.info("Allowing contact connection, below limit");
+			return true;
+		} else if (canAttemptToRaiseLimit(now)) {
+			LOG.info("Allowing contact connection, at limit");
+			return true;
+		} else {
+			LOG.info("Refusing contact connection, at limit");
+			return false;
+		}
+	}
+
 	@GuardedBy("lock")
 	private void considerRaisingConnectionLimit(long now) {
 		int stable = 0;
 		for (ConnectionRecord rec : connections) {
 			if (now - rec.timeOpened >= STABILITY_PERIOD_MS) stable++;
 		}
-		if (stable >= connectionLimit) {
+		if (stable > connectionLimit) {
 			LOG.info("Raising connection limit");
 			connectionLimit++;
 		}
@@ -162,6 +177,11 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter {
 		}
 	}
 
+	@GuardedBy("lock")
+	private boolean canAttemptToRaiseLimit(long now) {
+		return now - timeOfLastAttempt >= MIN_ATTEMPT_INTERVAL_MS;
+	}
+
 	private static final class ConnectionRecord {
 
 		private final DuplexTransportConnection connection;

bramble-core/src/test/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImplTest.java

@@ -18,8 +18,13 @@ public class BluetoothConnectionLimiterImplTest extends BrambleMockTestCase {
 
 	private final EventBus eventBus = context.mock(EventBus.class);
 	private final Clock clock = context.mock(Clock.class);
-	private final DuplexTransportConnection conn =
+
-			context.mock(DuplexTransportConnection.class);
+	private final DuplexTransportConnection conn1 =
+			context.mock(DuplexTransportConnection.class, "conn1");
+	private final DuplexTransportConnection conn2 =
+			context.mock(DuplexTransportConnection.class, "conn2");
+	private final DuplexTransportConnection conn3 =
+			context.mock(DuplexTransportConnection.class, "conn3");
 	private final TransportConnectionReader reader =
 			context.mock(TransportConnectionReader.class);
 	private final TransportConnectionWriter writer =
@@ -35,79 +40,103 @@ public class BluetoothConnectionLimiterImplTest extends BrambleMockTestCase {
 	}
 
 	@Test
-	public void testLimiterAllowsOneOutgoingConnection() {
+	public void testLimiterAllowsAttemptToRaiseLimitAtStartup()
-		expectGetCurrentTime(now);
-		assertTrue(limiter.canOpenContactConnection());
-
-		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
-
-		expectGetCurrentTime(now);
-		assertFalse(limiter.canOpenContactConnection());
-	}
-
-	@Test
-	public void testLimiterAllowsSecondIncomingConnection() throws Exception {
-		expectGetCurrentTime(now);
-		assertTrue(limiter.canOpenContactConnection());
-
-		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
-
-		expectGetCurrentTime(now);
-		assertFalse(limiter.canOpenContactConnection());
-
-		// The limiter allows a second incoming connection
-		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
-
-		// The limiter does not allow a third incoming connection
-		expectGetCurrentTime(now);
-		expectCloseConnection();
-		assertFalse(limiter.contactConnectionOpened(conn));
-	}
-
-	@Test
-	public void testLimiterAllowsSecondOutgoingConnectionWhenFirstIsStable() {
-		expectGetCurrentTime(now);
-		assertTrue(limiter.canOpenContactConnection());
-
-		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
-
-		// The first connection is not yet stable
-		expectGetCurrentTime(now + STABILITY_PERIOD_MS - 1);
-		assertFalse(limiter.canOpenContactConnection());
-
-		// The first connection is stable, so the limit is raised
-		expectGetCurrentTime(now + STABILITY_PERIOD_MS);
-		assertTrue(limiter.canOpenContactConnection());
-	}
-
-	@Test
-	public void testLimiterAllowsThirdIncomingConnectionWhenFirstTwoAreStable()
 			throws Exception {
+		// First outgoing connection is allowed - we're below the limit
 		expectGetCurrentTime(now);
 		assertTrue(limiter.canOpenContactConnection());
-
 		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
+		assertTrue(limiter.contactConnectionOpened(conn1));
 
+		// Second outgoing connection is allowed - it's time to try raising
+		// the limit
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn2));
+
+		// Third outgoing connection is not allowed - we're above the limit
 		expectGetCurrentTime(now);
 		assertFalse(limiter.canOpenContactConnection());
 
-		// The limiter allows a second incoming connection
+		// Third incoming connection is not allowed - we're above the limit
 		expectGetCurrentTime(now);
-		assertTrue(limiter.contactConnectionOpened(conn));
+		expectCloseConnection(conn3);
+		assertFalse(limiter.contactConnectionOpened(conn3));
+	}
 
-		// The limiter does not allow a third incoming connection
+	@Test
-		expectGetCurrentTime(now + STABILITY_PERIOD_MS - 1);
+	public void testLimiterAllowsThirdConnectionAfterFirstTwoAreClosed() {
-		expectCloseConnection();
+		// First outgoing connection is allowed - we're below the limit
-		assertFalse(limiter.contactConnectionOpened(conn));
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn1));
 
-		// The first two connections are stable, so the limit is raised
+		// Second outgoing connection is allowed - it's time to try raising
+		// the limit
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn2));
+
+		// Third outgoing connection is not allowed - we're above the limit
+		expectGetCurrentTime(now);
+		assertFalse(limiter.canOpenContactConnection());
+
+		// Close the first connection
+		limiter.connectionClosed(conn1, false);
+
+		// Third outgoing connection is not allowed - we're at the limit
+		expectGetCurrentTime(now);
+		assertFalse(limiter.canOpenContactConnection());
+
+		// Close the second connection
+		limiter.connectionClosed(conn2, false);
+
+		// Third outgoing connection is allowed - we're below the limit
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn3));
+	}
+
+	@Test
+	public void testLimiterRaisesLimitWhenConnectionsAreStable() {
+		// First outgoing connection is allowed - we're below the limit
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn1));
+
+		// Second outgoing connection is allowed - it's time to try raising
+		// the limit
+		expectGetCurrentTime(now);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now);
+		assertTrue(limiter.contactConnectionOpened(conn2));
+
+		// Third outgoing connection is not allowed - we're above the limit
+		expectGetCurrentTime(now);
+		assertFalse(limiter.canOpenContactConnection());
+
+		// Third outgoing connection is still not allowed - first two are
+		// stable but we're still at the limit
 		expectGetCurrentTime(now + STABILITY_PERIOD_MS);
-		assertTrue(limiter.contactConnectionOpened(conn));
+		assertFalse(limiter.canOpenContactConnection());
+
+		// Close the first connection
+		limiter.connectionClosed(conn1, false);
+
+		// Third outgoing connection is allowed - we're below the new limit
+		expectGetCurrentTime(now + STABILITY_PERIOD_MS);
+		assertTrue(limiter.canOpenContactConnection());
+		expectGetCurrentTime(now + STABILITY_PERIOD_MS);
+		assertTrue(limiter.contactConnectionOpened(conn3));
+
+		// Fourth outgoing connection is not allowed
+		expectGetCurrentTime(now + STABILITY_PERIOD_MS);
+		assertFalse(limiter.canOpenContactConnection());
 	}
 
 	private void expectGetCurrentTime(long now) {
@@ -117,7 +146,8 @@ public class BluetoothConnectionLimiterImplTest extends BrambleMockTestCase {
 		}});
 	}
 
-	private void expectCloseConnection() throws Exception {
+	private void expectCloseConnection(DuplexTransportConnection conn)
+			throws Exception {
 		context.checking(new Expectations() {{
 			oneOf(conn).getReader();
 			will(returnValue(reader));