From 13cca9ca61e63a5a12552c880cfe44c0fcecef8f Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 6 May 2020 16:20:44 +0100 Subject: [PATCH] Occasionally try to raise the limit by allowing an extra connection. --- .../bluetooth/BluetoothConnectionLimiter.java | 8 + .../BluetoothConnectionLimiterImpl.java | 52 ++++-- .../BluetoothConnectionLimiterImplTest.java | 158 +++++++++++------- 3 files changed, 138 insertions(+), 80 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiter.java b/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiter.java index 1e961d8e2..058efac13 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiter.java +++ b/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. */ diff --git a/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImpl.java index b5ddc5a89..e73ec4825 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImpl.java +++ b/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"); - return false; - } - considerRaisingConnectionLimit(clock.currentTimeMillis()); - if (connections.size() >= connectionLimit) { - LOG.info("Can't open contact connection due to limit"); + LOG.info("Refusing contact connection during key agreement"); return false; } else { - LOG.info("Can open contact connection"); - return true; + long now = clock.currentTimeMillis(); + 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); - if (connections.size() > connectionLimit) { - LOG.info("Refusing contact connection due to limit"); - accept = false; - } else { - LOG.info("Accepting contact connection"); + accept = isContactConnectionAllowedByLimit(now); + if (accept) { 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; diff --git a/bramble-core/src/test/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImplTest.java index e658fbe84..64d404225 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/plugin/bluetooth/BluetoothConnectionLimiterImplTest.java +++ b/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() { - 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() + public void testLimiterAllowsAttemptToRaiseLimitAtStartup() 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 - expectGetCurrentTime(now + STABILITY_PERIOD_MS - 1); - expectCloseConnection(); - assertFalse(limiter.contactConnectionOpened(conn)); + @Test + public void testLimiterAllowsThirdConnectionAfterFirstTwoAreClosed() { + // First outgoing connection is allowed - we're below the limit + 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));