From d3751fbeadd47b1559cdddae5b56bdb638a0f3ba Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 1 Jun 2020 14:23:08 +0100 Subject: [PATCH] Don't interrupt connections until priority is set. This maintains compatibility with older peers that don't know about priorities or transport preferences and will try to replace any connections we close. --- .../connection/ConnectionRegistryImpl.java | 61 ++++-------- .../ConnectionRegistryImplTest.java | 98 +++++++++++++++++-- 2 files changed, 107 insertions(+), 52 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionRegistryImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionRegistryImpl.java index b7337c7c0..192423b1c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionRegistryImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionRegistryImpl.java @@ -83,41 +83,16 @@ class ConnectionRegistryImpl implements ConnectionRegistry { if (incoming) LOG.info("Incoming connection registered: " + t); else LOG.info("Outgoing connection registered: " + t); } - List toInterrupt; - boolean firstConnection = false, interruptNewConnection = false; + boolean firstConnection; synchronized (lock) { List recs = contactConnections.get(c); if (recs == null) { recs = new ArrayList<>(); contactConnections.put(c, recs); } - if (recs.isEmpty()) { - toInterrupt = emptyList(); - firstConnection = true; - } else { - toInterrupt = new ArrayList<>(recs.size()); - for (ConnectionRecord rec : recs) { - int compare = compare(t, rec.transportId); - if (compare == -1) { - // The old connection is better than the new one - interruptNewConnection = true; - } else if (compare == 1 && !rec.interrupted) { - // The new connection is better than the old one - toInterrupt.add(rec.conn); - rec.interrupted = true; - } - } - } + firstConnection = recs.isEmpty(); recs.add(new ConnectionRecord(t, conn)); } - if (interruptNewConnection) { - LOG.info("Interrupting new connection"); - conn.interruptOutgoingSession(); - } - for (InterruptibleConnection old : toInterrupt) { - LOG.info("Interrupting old connection"); - old.interruptOutgoingSession(); - } eventBus.broadcast(new ConnectionOpenedEvent(c, t, incoming)); if (firstConnection) { LOG.info("Contact connected"); @@ -125,17 +100,6 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } } - private int compare(TransportId a, TransportId b) { - if (getBetterTransports(a).contains(b)) return -1; - else if (getBetterTransports(b).contains(a)) return 1; - else return 0; - } - - private List getBetterTransports(TransportId t) { - List better = betterTransports.get(t); - return better == null ? emptyList() : better; - } - @Override public void setPriority(ContactId c, TransportId t, InterruptibleConnection conn, Priority priority) { @@ -150,8 +114,9 @@ class ConnectionRegistryImpl implements ConnectionRegistry { if (rec.conn == conn) { // Store the priority of this connection rec.priority = priority; - } else if (rec.transportId.equals(t)) { - int compare = compare(priority, rec.priority); + } else if (rec.priority != null) { + int compare = compareConnections(t, priority, + rec.transportId, rec.priority); if (compare == -1) { // The old connection is better than the new one interruptNewConnection = true; @@ -173,8 +138,16 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } } - private int compare(Priority a, @Nullable Priority b) { - return b == null ? 0 : Bytes.compare(a.getNonce(), b.getNonce()); + private int compareConnections(TransportId tA, Priority pA, TransportId tB, + Priority pB) { + if (getBetterTransports(tA).contains(tB)) return -1; + if (getBetterTransports(tB).contains(tA)) return 1; + return tA.equals(tB) ? Bytes.compare(pA.getNonce(), pB.getNonce()) : 0; + } + + private List getBetterTransports(TransportId t) { + List better = betterTransports.get(t); + return better == null ? emptyList() : better; } @Override @@ -184,12 +157,12 @@ class ConnectionRegistryImpl implements ConnectionRegistry { if (incoming) LOG.info("Incoming connection unregistered: " + t); else LOG.info("Outgoing connection unregistered: " + t); } - boolean lastConnection = false; + boolean lastConnection; synchronized (lock) { List recs = contactConnections.get(c); if (recs == null || !recs.remove(new ConnectionRecord(t, conn))) throw new IllegalArgumentException(); - if (recs.isEmpty()) lastConnection = true; + lastConnection = recs.isEmpty(); } eventBus.broadcast(new ConnectionClosedEvent(c, t, incoming)); if (lastConnection) { diff --git a/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionRegistryImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionRegistryImplTest.java index 5c361c3d6..cb63a9fc7 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionRegistryImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionRegistryImplTest.java @@ -177,6 +177,79 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { c.getConnectedOrBetterContacts(transportId2)); } + @Test + public void testConnectionsAreNotInterruptedUnlessPriorityIsSet() { + // Prefer transport 2 to transport 1 + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue( + singletonList(new Pair<>(transportId2, transportId1)))); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); + + // Connect via transport 1 (worse than 2) and set priority to low + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + oneOf(eventBus).broadcast(with(any(ContactConnectedEvent.class))); + }}); + c.registerConnection(contactId1, transportId1, conn1, true); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + assertEquals(emptyList(), c.getConnectedContacts(transportId2)); + assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId2)); + + // Connect via transport 2 (better than 1) and set priority to high - + // the old connection should not be interrupted, despite using a worse + // transport, to remain compatible with old peers + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId2, conn2, true); + c.setPriority(contactId1, transportId2, conn2, high); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId2)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId2)); + + // Connect via transport 3 (no preference) and set priority to high - + // again, no interruptions are expected + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId3, conn3, true); + c.setPriority(contactId1, transportId3, conn3, high); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId2)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId2)); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId3)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId3)); + } + @Test public void testNewConnectionIsInterruptedIfOldConnectionUsesBetterTransport() { // Prefer transport 1 to transport 2 @@ -189,12 +262,13 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { ConnectionRegistry c = new ConnectionRegistryImpl(eventBus, pluginConfig); - // Connect via transport 1 (better than 2) + // Connect via transport 1 (better than 2) and set priority to low context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); oneOf(eventBus).broadcast(with(any(ContactConnectedEvent.class))); }}); c.registerConnection(contactId1, transportId1, conn1, true); + c.setPriority(contactId1, transportId1, conn1, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -208,13 +282,15 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId2)); - // Connect via transport 2 (worse than 1) - the new connection should - // be interrupted + // Connect via transport 2 (worse than 1) and set priority to high - + // the new connection should be interrupted because it uses a worse + // transport context.checking(new Expectations() {{ oneOf(conn2).interruptOutgoingSession(); oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); c.registerConnection(contactId1, transportId2, conn2, true); + c.setPriority(contactId1, transportId2, conn2, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -227,11 +303,13 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId2)); - // Connect via transport 3 (no preference) + // Connect via transport 3 (no preference) and set priority to low - + // no further interruptions context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); c.registerConnection(contactId1, transportId3, conn3, true); + c.setPriority(contactId1, transportId3, conn3, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -285,12 +363,13 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { ConnectionRegistry c = new ConnectionRegistryImpl(eventBus, pluginConfig); - // Connect via transport 1 (worse than 2) + // Connect via transport 1 (worse than 2) and set priority to high context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); oneOf(eventBus).broadcast(with(any(ContactConnectedEvent.class))); }}); c.registerConnection(contactId1, transportId1, conn1, true); + c.setPriority(contactId1, transportId1, conn1, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -301,13 +380,15 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(emptyList(), c.getConnectedContacts(transportId2)); assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId2)); - // Connect via transport 2 (better than 1) - the old connection should - // be interrupted + // Connect via transport 2 (better than 1) and set priority to low - + // the old connection should be interrupted because it uses a worse + // transport context.checking(new Expectations() {{ oneOf(conn1).interruptOutgoingSession(); oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); c.registerConnection(contactId1, transportId2, conn2, true); + c.setPriority(contactId1, transportId2, conn2, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -320,7 +401,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId2)); - // Connect via transport 3 (no preference) + // Connect via transport 3 (no preference) and set priority to high - + // no further interruptions context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }});