From 9771825c4568de3d04d2a4e19739176a01c0203c Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 7 May 2020 16:10:29 +0100 Subject: [PATCH] Back off between attempts to raise connection limit. --- .../bluetooth/BluetoothConnectionLimiter.java | 9 +- .../BluetoothConnectionLimiterImpl.java | 23 ++- .../BluetoothConnectionLimiterImplTest.java | 135 ++++++++++++------ 3 files changed, 116 insertions(+), 51 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 058efac13..d16d9ed2c 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 @@ -19,7 +19,14 @@ interface BluetoothConnectionLimiter { * 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); + long MIN_ATTEMPT_INTERVAL_MS = MINUTES.toMillis(2); + + /** + * The maximum exponent to use when backing off between attempts to raise + * the connection limit. The maximum interval between attempts is + * MIN_ATTEMPT_INTERVAL_MS * (1L << MAX_ATTEMPT_BACKOFF) =~ 34 hours. + */ + int MAX_ATTEMPT_BACKOFF = 10; /** * 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 e73ec4825..423a10850 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 @@ -16,6 +16,7 @@ import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; +import static java.lang.Math.min; import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; @@ -38,9 +39,10 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter { @GuardedBy("lock") private boolean keyAgreementInProgress = false; @GuardedBy("lock") - private int connectionLimit = 1; + private int connectionLimit = 1, attemptBackoff = 0; @GuardedBy("lock") - private long timeOfLastAttempt = 0; + private long timeOfLastAttempt = 0, + attemptInterval = MIN_ATTEMPT_INTERVAL_MS; @Inject BluetoothConnectionLimiterImpl(EventBus eventBus, Clock clock) { @@ -123,10 +125,14 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter { public void connectionClosed(DuplexTransportConnection conn, boolean exception) { synchronized (lock) { + int numConnections = connections.size(); Iterator it = connections.iterator(); while (it.hasNext()) { if (it.next().connection == conn) { it.remove(); + if (exception && numConnections > connectionLimit) { + connectionFailedAboveLimit(); + } break; } } @@ -152,7 +158,7 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter { } else if (connections.size() < connectionLimit) { LOG.info("Allowing contact connection, below limit"); return true; - } else if (canAttemptToRaiseLimit(now)) { + } else if (now - timeOfLastAttempt >= attemptInterval) { LOG.info("Allowing contact connection, at limit"); return true; } else { @@ -170,6 +176,8 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter { if (stable > connectionLimit) { LOG.info("Raising connection limit"); connectionLimit++; + attemptBackoff = 0; + attemptInterval = MIN_ATTEMPT_INTERVAL_MS; } if (LOG.isLoggable(INFO)) { LOG.info(stable + " connections are stable, limit is " @@ -178,8 +186,13 @@ class BluetoothConnectionLimiterImpl implements BluetoothConnectionLimiter { } @GuardedBy("lock") - private boolean canAttemptToRaiseLimit(long now) { - return now - timeOfLastAttempt >= MIN_ATTEMPT_INTERVAL_MS; + private void connectionFailedAboveLimit() { + long now = clock.currentTimeMillis(); + if (now - timeOfLastAttempt < STABILITY_PERIOD_MS) { + LOG.info("Connection failed above limit, increasing interval"); + attemptBackoff = min(attemptBackoff + 1, MAX_ATTEMPT_BACKOFF); + attemptInterval = MIN_ATTEMPT_INTERVAL_MS * (1L << attemptBackoff); + } } private static final class ConnectionRecord { 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 64d404225..c389bfe53 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 @@ -4,12 +4,17 @@ import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.plugin.TransportConnectionReader; import org.briarproject.bramble.api.plugin.TransportConnectionWriter; import org.briarproject.bramble.api.plugin.duplex.DuplexTransportConnection; +import org.briarproject.bramble.api.sync.event.CloseSyncConnectionsEvent; import org.briarproject.bramble.api.system.Clock; import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.SettableClock; import org.jmock.Expectations; import org.junit.Before; import org.junit.Test; +import java.util.concurrent.atomic.AtomicLong; + +import static org.briarproject.bramble.plugin.bluetooth.BluetoothConnectionLimiter.MIN_ATTEMPT_INTERVAL_MS; import static org.briarproject.bramble.plugin.bluetooth.BluetoothConnectionLimiter.STABILITY_PERIOD_MS; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -17,7 +22,6 @@ import static org.junit.Assert.assertTrue; public class BluetoothConnectionLimiterImplTest extends BrambleMockTestCase { private final EventBus eventBus = context.mock(EventBus.class); - private final Clock clock = context.mock(Clock.class); private final DuplexTransportConnection conn1 = context.mock(DuplexTransportConnection.class, "conn1"); @@ -32,117 +36,158 @@ public class BluetoothConnectionLimiterImplTest extends BrambleMockTestCase { private final long now = System.currentTimeMillis(); + private AtomicLong time; private BluetoothConnectionLimiter limiter; @Before public void setUp() { + time = new AtomicLong(now); + Clock clock = new SettableClock(time); limiter = new BluetoothConnectionLimiterImpl(eventBus, clock); } + @Test + public void testLimiterDoesNotAllowContactConnectionsDuringKeyAgreement() { + assertTrue(limiter.canOpenContactConnection()); + + expectCloseSyncConnectionsEvent(); + limiter.keyAgreementStarted(); + + assertFalse(limiter.canOpenContactConnection()); + + limiter.keyAgreementEnded(); + + assertTrue(limiter.canOpenContactConnection()); + } + @Test public void testLimiterAllowsAttemptToRaiseLimitAtStartup() throws Exception { - // First outgoing connection is allowed - we're below the limit - expectGetCurrentTime(now); + // First outgoing connection is allowed - we're below the limit of 1 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn1)); // Second outgoing connection is allowed - it's time to try raising - // the limit - expectGetCurrentTime(now); + // the limit to 2 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn2)); - // Third outgoing connection is not allowed - we're above the limit - expectGetCurrentTime(now); + // Third outgoing connection is not allowed - we're above the limit of 1 assertFalse(limiter.canOpenContactConnection()); - // Third incoming connection is not allowed - we're above the limit - expectGetCurrentTime(now); + // Third incoming connection is not allowed - we're above the limit of 1 expectCloseConnection(conn3); assertFalse(limiter.contactConnectionOpened(conn3)); } @Test public void testLimiterAllowsThirdConnectionAfterFirstTwoAreClosed() { - // First outgoing connection is allowed - we're below the limit - expectGetCurrentTime(now); + // First outgoing connection is allowed - we're below the limit of 1 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn1)); // Second outgoing connection is allowed - it's time to try raising - // the limit - expectGetCurrentTime(now); + // the limit to 2 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn2)); - // Third outgoing connection is not allowed - we're above the limit - expectGetCurrentTime(now); + // Third outgoing connection is not allowed - we're above the limit of 1 assertFalse(limiter.canOpenContactConnection()); // Close the first connection limiter.connectionClosed(conn1, false); - // Third outgoing connection is not allowed - we're at the limit - expectGetCurrentTime(now); + // Third outgoing connection is not allowed - we're at the limit of 1 assertFalse(limiter.canOpenContactConnection()); // Close the second connection limiter.connectionClosed(conn2, false); - // Third outgoing connection is allowed - we're below the limit - expectGetCurrentTime(now); + // Third outgoing connection is allowed - we're below the limit of 1 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); + // First outgoing connection is allowed - we're below the limit of 1 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn1)); // Second outgoing connection is allowed - it's time to try raising - // the limit - expectGetCurrentTime(now); + // the limit to 2 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now); assertTrue(limiter.contactConnectionOpened(conn2)); - // Third outgoing connection is not allowed - we're above the limit - expectGetCurrentTime(now); + // Third outgoing connection is not allowed - we're above the limit of 1 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); + // Time passes + time.set(now + STABILITY_PERIOD_MS); + + // Third outgoing connection is still not allowed - first two are now + // stable so limit is raised to 2, but we're already at the new limit assertFalse(limiter.canOpenContactConnection()); - // Close the first connection - limiter.connectionClosed(conn1, false); + // Time passes + time.set(now + MIN_ATTEMPT_INTERVAL_MS); - // Third outgoing connection is allowed - we're below the new limit - expectGetCurrentTime(now + STABILITY_PERIOD_MS); + // Third outgoing connection is allowed - it's time to try raising + // the limit to 3 assertTrue(limiter.canOpenContactConnection()); - expectGetCurrentTime(now + STABILITY_PERIOD_MS); assertTrue(limiter.contactConnectionOpened(conn3)); - // Fourth outgoing connection is not allowed - expectGetCurrentTime(now + STABILITY_PERIOD_MS); + // Fourth outgoing connection is not allowed - we're above the limit + // of 2 assertFalse(limiter.canOpenContactConnection()); } - private void expectGetCurrentTime(long now) { + @Test + public void testLimiterIncreasesIntervalWhenConnectionFailsAboveLimit() { + // First outgoing connection is allowed - we're below the limit of 1 + assertTrue(limiter.canOpenContactConnection()); + assertTrue(limiter.contactConnectionOpened(conn1)); + + // Time passes + time.set(now + 1); + + // Second outgoing connection is allowed - it's time to try raising + // the limit to 2 + assertTrue(limiter.canOpenContactConnection()); + assertTrue(limiter.contactConnectionOpened(conn2)); + + // Time passes - the first connection is stable, the second isn't + time.set(now + STABILITY_PERIOD_MS); + + // First connection fails. The second connection isn't stable yet, so + // the limiter considers this a failed attempt and doubles the interval + // between attempts + limiter.connectionClosed(conn1, true); + + // Third outgoing connection is not allowed - we're still at the limit + // of 1 + assertFalse(limiter.canOpenContactConnection()); + + // Time passes - nearly time for the second attempt + time.set(now + MIN_ATTEMPT_INTERVAL_MS * 2); + + // Third outgoing connection is not allowed - we're still at the limit + // of 1 + assertFalse(limiter.canOpenContactConnection()); + + // Time passes - now it's time for the second attempt + time.set(now + 1 + MIN_ATTEMPT_INTERVAL_MS * 2); + + // Third outgoing connection is allowed - it's time to try raising the + // limit to 2 again + assertTrue(limiter.canOpenContactConnection()); + assertTrue(limiter.contactConnectionOpened(conn3)); + } + + private void expectCloseSyncConnectionsEvent() { context.checking(new Expectations() {{ - oneOf(clock).currentTimeMillis(); - will(returnValue(now)); + oneOf(eventBus).broadcast(with(any( + CloseSyncConnectionsEvent.class))); }}); }