From 9e6d67f13d77749feb4ae4e75d9c2d0e756b64b7 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Tue, 12 May 2020 17:42:33 +0100 Subject: [PATCH 01/21] Handle interrupts that occur before the outgoing session starts. --- .../connection/DuplexSyncConnection.java | 29 ++++++++++++++++--- .../IncomingDuplexSyncConnection.java | 6 ++-- .../OutgoingDuplexSyncConnection.java | 6 ++-- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java index 0ed62ab0b..55be56812 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.concurrent.Executor; import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; @@ -33,8 +34,30 @@ abstract class DuplexSyncConnection extends SyncConnection { final TransportConnectionWriter writer; final TransportProperties remote; + private final Object interruptLock = new Object(); + + @GuardedBy("interruptLock") @Nullable - volatile SyncSession outgoingSession = null; + private SyncSession outgoingSession = null; + @GuardedBy("interruptLock") + private boolean interruptWaiting = false; + + void interruptOutgoingSession() { + synchronized (interruptLock) { + if (outgoingSession == null) interruptWaiting = true; + else outgoingSession.interrupt(); + } + } + + void setOutgoingSession(SyncSession outgoingSession) { + synchronized (interruptLock) { + this.outgoingSession = outgoingSession; + if (interruptWaiting) { + outgoingSession.interrupt(); + interruptWaiting = false; + } + } + } DuplexSyncConnection(KeyManager keyManager, ConnectionRegistry connectionRegistry, @@ -57,9 +80,7 @@ abstract class DuplexSyncConnection extends SyncConnection { void onReadError(boolean recognised) { disposeOnError(reader, recognised); disposeOnError(writer); - // Interrupt the outgoing session so it finishes - SyncSession out = outgoingSession; - if (out != null) out.interrupt(); + interruptOutgoingSession(); } void onWriteError() { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java index 382b17a50..ba780cbc5 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java @@ -68,9 +68,7 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection // Create and run the incoming session createIncomingSession(ctx, reader).run(); reader.dispose(false, true); - // Interrupt the outgoing session so it finishes cleanly - SyncSession out = outgoingSession; - if (out != null) out.interrupt(); + interruptOutgoingSession(); } catch (DbException | IOException e) { logException(LOG, WARNING, e); onReadError(true); @@ -91,7 +89,7 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection try { // Create and run the outgoing session SyncSession out = createDuplexOutgoingSession(ctx, writer); - outgoingSession = out; + setOutgoingSession(out); out.run(); writer.dispose(false); } catch (IOException e) { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java index 0c8f464fb..ed6d21d05 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java @@ -60,7 +60,7 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection try { // Create and run the outgoing session SyncSession out = createDuplexOutgoingSession(ctx, writer); - outgoingSession = out; + setOutgoingSession(out); out.run(); writer.dispose(false); } catch (IOException e) { @@ -104,9 +104,7 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection // Create and run the incoming session createIncomingSession(ctx, reader).run(); reader.dispose(false, true); - // Interrupt the outgoing session so it finishes cleanly - SyncSession out = outgoingSession; - if (out != null) out.interrupt(); + interruptOutgoingSession(); } catch (DbException | IOException e) { logException(LOG, WARNING, e); onReadError(); From ee9c77104555b2caa20466486c4b2f9133a6ed9e Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 11 May 2020 16:27:36 +0100 Subject: [PATCH 02/21] Add priority record for choosing between redundant connections. --- .../bramble/api/sync/Priority.java | 23 +++++++++++++ .../bramble/api/sync/RecordTypes.java | 1 + .../bramble/api/sync/SyncConstants.java | 6 ++++ .../bramble/api/sync/SyncRecordReader.java | 4 +++ .../bramble/api/sync/SyncRecordWriter.java | 2 ++ .../bramble/sync/SyncRecordReaderImpl.java | 24 ++++++++++++- .../bramble/sync/SyncRecordWriterImpl.java | 8 +++++ .../sync/SyncRecordReaderImplTest.java | 34 +++++++++++++++++++ 8 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/sync/Priority.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/Priority.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/Priority.java new file mode 100644 index 000000000..44a2169b7 --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/Priority.java @@ -0,0 +1,23 @@ +package org.briarproject.bramble.api.sync; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import javax.annotation.concurrent.Immutable; + +/** + * A record containing a nonce for choosing between redundant sessions. + */ +@Immutable +@NotNullByDefault +public class Priority { + + private final byte[] nonce; + + public Priority(byte[] nonce) { + this.nonce = nonce; + } + + public byte[] getNonce() { + return nonce; + } +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/RecordTypes.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/RecordTypes.java index 168b4c9ef..7fdbe0af7 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/RecordTypes.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/RecordTypes.java @@ -10,4 +10,5 @@ public interface RecordTypes { byte OFFER = 2; byte REQUEST = 3; byte VERSIONS = 4; + byte PRIORITY = 5; } diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncConstants.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncConstants.java index c430ac05f..2e9241bed 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncConstants.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncConstants.java @@ -49,4 +49,10 @@ public interface SyncConstants { * simultaneously. */ int MAX_SUPPORTED_VERSIONS = 10; + + /** + * The length of the priority nonce used for choosing between redundant + * connections. + */ + int PRIORITY_NONCE_BYTES = 16; } diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordReader.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordReader.java index 55650e407..c0f9b947a 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordReader.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordReader.java @@ -28,4 +28,8 @@ public interface SyncRecordReader { boolean hasVersions() throws IOException; Versions readVersions() throws IOException; + + boolean hasPriority() throws IOException; + + Priority readPriority() throws IOException; } diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordWriter.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordWriter.java index bdeca0cf8..75d4b8401 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordWriter.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncRecordWriter.java @@ -17,5 +17,7 @@ public interface SyncRecordWriter { void writeVersions(Versions v) throws IOException; + void writePriority(Priority p) throws IOException; + void flush() throws IOException; } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordReaderImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordReaderImpl.java index 7ea0f6a41..8d8857460 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordReaderImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordReaderImpl.java @@ -11,6 +11,7 @@ import org.briarproject.bramble.api.sync.Message; import org.briarproject.bramble.api.sync.MessageFactory; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.api.sync.Offer; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.api.sync.Request; import org.briarproject.bramble.api.sync.SyncRecordReader; import org.briarproject.bramble.api.sync.Versions; @@ -26,10 +27,12 @@ import javax.annotation.concurrent.NotThreadSafe; import static org.briarproject.bramble.api.sync.RecordTypes.ACK; import static org.briarproject.bramble.api.sync.RecordTypes.MESSAGE; import static org.briarproject.bramble.api.sync.RecordTypes.OFFER; +import static org.briarproject.bramble.api.sync.RecordTypes.PRIORITY; import static org.briarproject.bramble.api.sync.RecordTypes.REQUEST; import static org.briarproject.bramble.api.sync.RecordTypes.VERSIONS; import static org.briarproject.bramble.api.sync.SyncConstants.MAX_SUPPORTED_VERSIONS; import static org.briarproject.bramble.api.sync.SyncConstants.MESSAGE_HEADER_LENGTH; +import static org.briarproject.bramble.api.sync.SyncConstants.PRIORITY_NONCE_BYTES; import static org.briarproject.bramble.api.sync.SyncConstants.PROTOCOL_VERSION; @NotThreadSafe @@ -48,7 +51,7 @@ class SyncRecordReaderImpl implements SyncRecordReader { private static boolean isKnownRecordType(byte type) { return type == ACK || type == MESSAGE || type == OFFER || - type == REQUEST || type == VERSIONS; + type == REQUEST || type == VERSIONS || type == PRIORITY; } private final MessageFactory messageFactory; @@ -174,4 +177,23 @@ class SyncRecordReaderImpl implements SyncRecordReader { nextRecord = null; return supported; } + + @Override + public boolean hasPriority() throws IOException { + return !eof() && getNextRecordType() == PRIORITY; + } + + @Override + public Priority readPriority() throws IOException { + if (!hasPriority()) throw new FormatException(); + return new Priority(readNonce()); + } + + private byte[] readNonce() throws IOException { + if (nextRecord == null) throw new AssertionError(); + byte[] payload = nextRecord.getPayload(); + if (payload.length != PRIORITY_NONCE_BYTES) throw new FormatException(); + nextRecord = null; + return payload; + } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordWriterImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordWriterImpl.java index 118cc51cd..d2b4e73f1 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordWriterImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncRecordWriterImpl.java @@ -8,6 +8,7 @@ import org.briarproject.bramble.api.sync.Message; import org.briarproject.bramble.api.sync.MessageFactory; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.api.sync.Offer; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.api.sync.Request; import org.briarproject.bramble.api.sync.SyncRecordWriter; import org.briarproject.bramble.api.sync.Versions; @@ -20,6 +21,7 @@ import javax.annotation.concurrent.NotThreadSafe; import static org.briarproject.bramble.api.sync.RecordTypes.ACK; import static org.briarproject.bramble.api.sync.RecordTypes.MESSAGE; import static org.briarproject.bramble.api.sync.RecordTypes.OFFER; +import static org.briarproject.bramble.api.sync.RecordTypes.PRIORITY; import static org.briarproject.bramble.api.sync.RecordTypes.REQUEST; import static org.briarproject.bramble.api.sync.RecordTypes.VERSIONS; import static org.briarproject.bramble.api.sync.SyncConstants.PROTOCOL_VERSION; @@ -73,6 +75,12 @@ class SyncRecordWriterImpl implements SyncRecordWriter { writeRecord(VERSIONS); } + @Override + public void writePriority(Priority p) throws IOException { + writer.writeRecord( + new Record(PROTOCOL_VERSION, PRIORITY, p.getNonce())); + } + @Override public void flush() throws IOException { writer.flush(); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/sync/SyncRecordReaderImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/sync/SyncRecordReaderImplTest.java index 667c2b557..740451837 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/sync/SyncRecordReaderImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/sync/SyncRecordReaderImplTest.java @@ -8,6 +8,7 @@ import org.briarproject.bramble.api.record.RecordReader; import org.briarproject.bramble.api.sync.Ack; import org.briarproject.bramble.api.sync.MessageFactory; import org.briarproject.bramble.api.sync.Offer; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.api.sync.Request; import org.briarproject.bramble.api.sync.SyncRecordReader; import org.briarproject.bramble.api.sync.Versions; @@ -24,11 +25,14 @@ import javax.annotation.Nullable; import static org.briarproject.bramble.api.record.Record.MAX_RECORD_PAYLOAD_BYTES; import static org.briarproject.bramble.api.sync.RecordTypes.ACK; import static org.briarproject.bramble.api.sync.RecordTypes.OFFER; +import static org.briarproject.bramble.api.sync.RecordTypes.PRIORITY; import static org.briarproject.bramble.api.sync.RecordTypes.REQUEST; import static org.briarproject.bramble.api.sync.RecordTypes.VERSIONS; import static org.briarproject.bramble.api.sync.SyncConstants.MAX_MESSAGE_IDS; import static org.briarproject.bramble.api.sync.SyncConstants.MAX_SUPPORTED_VERSIONS; +import static org.briarproject.bramble.api.sync.SyncConstants.PRIORITY_NONCE_BYTES; import static org.briarproject.bramble.api.sync.SyncConstants.PROTOCOL_VERSION; +import static org.briarproject.bramble.test.TestUtils.getRandomBytes; import static org.briarproject.bramble.test.TestUtils.getRandomId; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -119,6 +123,31 @@ public class SyncRecordReaderImplTest extends BrambleMockTestCase { reader.readVersions(); } + @Test(expected = FormatException.class) + public void testFormatExceptionIfPriorityNonceIsTooSmall() + throws Exception { + expectReadRecord(createPriority(PRIORITY_NONCE_BYTES - 1)); + + reader.readPriority(); + } + + @Test(expected = FormatException.class) + public void testFormatExceptionIfPriorityNonceIsTooLarge() + throws Exception { + expectReadRecord(createPriority(PRIORITY_NONCE_BYTES + 1)); + + reader.readPriority(); + } + + @Test + public void testNoFormatExceptionIfPriorityNonceIsCorrectSize() + throws Exception { + expectReadRecord(createPriority(PRIORITY_NONCE_BYTES)); + + Priority priority = reader.readPriority(); + assertEquals(PRIORITY_NONCE_BYTES, priority.getNonce().length); + } + @Test public void testEofReturnsTrueWhenAtEndOfStream() throws Exception { expectReadRecord(createAck()); @@ -173,6 +202,11 @@ public class SyncRecordReaderImplTest extends BrambleMockTestCase { return new Record(PROTOCOL_VERSION, VERSIONS, payload); } + private Record createPriority(int nonceBytes) { + byte[] payload = getRandomBytes(nonceBytes); + return new Record(PROTOCOL_VERSION, PRIORITY, payload); + } + private byte[] createPayload() throws Exception { ByteArrayOutputStream payload = new ByteArrayOutputStream(); while (payload.size() + UniqueId.LENGTH <= MAX_RECORD_PAYLOAD_BYTES) { From 1b2b50d91b882ec70421058752a514a99ab53737 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Tue, 12 May 2020 21:18:56 +0100 Subject: [PATCH 03/21] Exchange priority records and close redundant connections. --- .../bramble/api/sync/PriorityHandler.java | 13 +++ .../bramble/api/sync/SyncSessionFactory.java | 8 +- .../bramble/connection/ConnectionChooser.java | 22 +++++ .../connection/ConnectionChooserImpl.java | 97 +++++++++++++++++++ .../connection/ConnectionManagerImpl.java | 12 ++- .../bramble/connection/ConnectionModule.java | 7 ++ .../connection/DuplexSyncConnection.java | 12 ++- .../IncomingDuplexSyncConnection.java | 16 ++- .../IncomingSimplexSyncConnection.java | 6 +- .../OutgoingDuplexSyncConnection.java | 35 +++++-- .../bramble/connection/SyncConnection.java | 7 +- .../bramble/sync/DuplexOutgoingSession.java | 9 +- .../bramble/sync/IncomingSession.java | 9 +- .../bramble/sync/SyncSessionFactoryImpl.java | 14 ++- 14 files changed, 237 insertions(+), 30 deletions(-) create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/sync/PriorityHandler.java create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/PriorityHandler.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/PriorityHandler.java new file mode 100644 index 000000000..57b2bcf98 --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/PriorityHandler.java @@ -0,0 +1,13 @@ +package org.briarproject.bramble.api.sync; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +/** + * An interface for handling a {@link Priority} record received by an + * incoming {@link SyncSession}. + */ +@NotNullByDefault +public interface PriorityHandler { + + void handle(Priority p); +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncSessionFactory.java b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncSessionFactory.java index 216f29398..2107c9a1f 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncSessionFactory.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/sync/SyncSessionFactory.java @@ -6,14 +6,18 @@ import org.briarproject.bramble.api.transport.StreamWriter; import java.io.InputStream; +import javax.annotation.Nullable; + @NotNullByDefault public interface SyncSessionFactory { - SyncSession createIncomingSession(ContactId c, InputStream in); + SyncSession createIncomingSession(ContactId c, InputStream in, + PriorityHandler handler); SyncSession createSimplexOutgoingSession(ContactId c, int maxLatency, StreamWriter streamWriter); SyncSession createDuplexOutgoingSession(ContactId c, int maxLatency, - int maxIdleTime, StreamWriter streamWriter); + int maxIdleTime, StreamWriter streamWriter, + @Nullable Priority priority); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java new file mode 100644 index 000000000..da26c904f --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java @@ -0,0 +1,22 @@ +package org.briarproject.bramble.connection; + +import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.plugin.TransportId; +import org.briarproject.bramble.api.sync.Priority; + +@NotNullByDefault +interface ConnectionChooser { + + /** + * Adds the given connection to the chooser with the given priority. + */ + void addConnection(ContactId c, TransportId t, DuplexSyncConnection conn, + Priority p); + + /** + * Removes the given connection from the chooser. + */ + void removeConnection(ContactId c, TransportId t, + DuplexSyncConnection conn); +} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java new file mode 100644 index 000000000..e3c64e449 --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java @@ -0,0 +1,97 @@ +package org.briarproject.bramble.connection; + +import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.plugin.TransportId; +import org.briarproject.bramble.api.sync.Priority; + +import java.util.HashMap; +import java.util.Map; +import java.util.logging.Logger; + +import javax.annotation.concurrent.GuardedBy; +import javax.inject.Inject; + +import static java.util.logging.Logger.getLogger; +import static org.briarproject.bramble.api.Bytes.compare; + +@NotNullByDefault +class ConnectionChooserImpl implements ConnectionChooser { + + private static final Logger LOG = + getLogger(ConnectionChooserImpl.class.getName()); + + private final Object lock = new Object(); + @GuardedBy("lock") + private final Map bestConnections = new HashMap<>(); + + @Inject + ConnectionChooserImpl() { + } + + @Override + public void addConnection(ContactId c, TransportId t, + DuplexSyncConnection conn, Priority p) { + synchronized (lock) { + Key k = new Key(c, t); + Value best = bestConnections.get(k); + if (best == null) { + bestConnections.put(k, new Value(conn, p)); + } else if (compare(p.getNonce(), best.priority.getNonce()) > 0) { + LOG.info("Found a better connection"); + bestConnections.put(k, new Value(conn, p)); + best.connection.interruptOutgoingSession(); + } + LOG.info("Already have a better connection"); + conn.interruptOutgoingSession(); + } + } + + @Override + public void removeConnection(ContactId c, TransportId t, + DuplexSyncConnection conn) { + synchronized (lock) { + Key k = new Key(c, t); + Value best = bestConnections.get(k); + if (best.connection == conn) bestConnections.remove(k); + } + } + + private static class Key { + + private final ContactId contactId; + private final TransportId transportId; + + private Key(ContactId contactId, TransportId transportId) { + this.contactId = contactId; + this.transportId = transportId; + } + + @Override + public int hashCode() { + return contactId.hashCode(); + } + + @Override + public boolean equals(Object o) { + if (o instanceof Key) { + Key k = (Key) o; + return contactId.equals(k.contactId) && + transportId.equals(k.transportId); + } else { + return false; + } + } + } + + private static class Value { + + private final DuplexSyncConnection connection; + private final Priority priority; + + private Value(DuplexSyncConnection connection, Priority priority) { + this.connection = connection; + this.priority = priority; + } + } +} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java index 7df9b1074..71914cb3c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java @@ -18,6 +18,7 @@ import org.briarproject.bramble.api.transport.KeyManager; import org.briarproject.bramble.api.transport.StreamReaderFactory; import org.briarproject.bramble.api.transport.StreamWriterFactory; +import java.security.SecureRandom; import java.util.concurrent.Executor; import javax.annotation.concurrent.Immutable; @@ -36,6 +37,8 @@ class ConnectionManagerImpl implements ConnectionManager { private final ContactExchangeManager contactExchangeManager; private final ConnectionRegistry connectionRegistry; private final TransportPropertyManager transportPropertyManager; + private final ConnectionChooser connectionChooser; + private final SecureRandom secureRandom; @Inject ConnectionManagerImpl(@IoExecutor Executor ioExecutor, @@ -45,7 +48,8 @@ class ConnectionManagerImpl implements ConnectionManager { HandshakeManager handshakeManager, ContactExchangeManager contactExchangeManager, ConnectionRegistry connectionRegistry, - TransportPropertyManager transportPropertyManager) { + TransportPropertyManager transportPropertyManager, + ConnectionChooser connectionChooser, SecureRandom secureRandom) { this.ioExecutor = ioExecutor; this.keyManager = keyManager; this.streamReaderFactory = streamReaderFactory; @@ -55,6 +59,8 @@ class ConnectionManagerImpl implements ConnectionManager { this.contactExchangeManager = contactExchangeManager; this.connectionRegistry = connectionRegistry; this.transportPropertyManager = transportPropertyManager; + this.connectionChooser = connectionChooser; + this.secureRandom = secureRandom; } @@ -72,7 +78,7 @@ class ConnectionManagerImpl implements ConnectionManager { ioExecutor.execute(new IncomingDuplexSyncConnection(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager, ioExecutor, - t, d)); + connectionChooser, t, d)); } @Override @@ -97,7 +103,7 @@ class ConnectionManagerImpl implements ConnectionManager { ioExecutor.execute(new OutgoingDuplexSyncConnection(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager, ioExecutor, - c, t, d)); + connectionChooser, secureRandom, c, t, d)); } @Override diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java index e6b8fee3a..08f299352 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java @@ -23,4 +23,11 @@ public class ConnectionModule { ConnectionRegistryImpl connectionRegistry) { return connectionRegistry; } + + @Provides + @Singleton + ConnectionChooser provideConnectionChooser( + ConnectionChooserImpl connectionChooser) { + return connectionChooser; + } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java index 55be56812..2f5415d26 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java @@ -9,6 +9,7 @@ import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.duplex.DuplexTransportConnection; import org.briarproject.bramble.api.properties.TransportProperties; import org.briarproject.bramble.api.properties.TransportPropertyManager; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.api.sync.SyncSession; import org.briarproject.bramble.api.sync.SyncSessionFactory; import org.briarproject.bramble.api.transport.KeyManager; @@ -29,6 +30,7 @@ import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; abstract class DuplexSyncConnection extends SyncConnection { final Executor ioExecutor; + final ConnectionChooser connectionChooser; final TransportId transportId; final TransportConnectionReader reader; final TransportConnectionWriter writer; @@ -65,12 +67,13 @@ abstract class DuplexSyncConnection extends SyncConnection { StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, TransportId transportId, - DuplexTransportConnection connection) { + Executor ioExecutor, ConnectionChooser connectionChooser, + TransportId transportId, DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager); this.ioExecutor = ioExecutor; + this.connectionChooser = connectionChooser; this.transportId = transportId; reader = connection.getReader(); writer = connection.getWriter(); @@ -89,11 +92,12 @@ abstract class DuplexSyncConnection extends SyncConnection { } SyncSession createDuplexOutgoingSession(StreamContext ctx, - TransportConnectionWriter w) throws IOException { + TransportConnectionWriter w, @Nullable Priority priority) + throws IOException { StreamWriter streamWriter = streamWriterFactory.createStreamWriter( w.getOutputStream(), ctx); ContactId c = requireNonNull(ctx.getContactId()); return syncSessionFactory.createDuplexOutgoingSession(c, - w.getMaxLatency(), w.getMaxIdleTime(), streamWriter); + w.getMaxLatency(), w.getMaxIdleTime(), streamWriter, priority); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java index ba780cbc5..5b72532e2 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java @@ -7,6 +7,7 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.duplex.DuplexTransportConnection; import org.briarproject.bramble.api.properties.TransportPropertyManager; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.SyncSession; import org.briarproject.bramble.api.sync.SyncSessionFactory; import org.briarproject.bramble.api.transport.KeyManager; @@ -30,11 +31,12 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, TransportId transportId, - DuplexTransportConnection connection) { + Executor ioExecutor, ConnectionChooser connectionChooser, + TransportId transportId, DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, - transportPropertyManager, ioExecutor, transportId, connection); + transportPropertyManager, ioExecutor, connectionChooser, + transportId, connection); } @Override @@ -65,8 +67,11 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection // Store any transport properties discovered from the connection transportPropertyManager.addRemotePropertiesFromConnection( contactId, transportId, remote); + // Add the connection to the chooser when we receive its priority + PriorityHandler handler = p -> connectionChooser.addConnection( + contactId, transportId, this, p); // Create and run the incoming session - createIncomingSession(ctx, reader).run(); + createIncomingSession(ctx, reader, handler).run(); reader.dispose(false, true); interruptOutgoingSession(); } catch (DbException | IOException e) { @@ -75,6 +80,7 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection } finally { connectionRegistry.unregisterConnection(contactId, transportId, true); + connectionChooser.removeConnection(contactId, transportId, this); } } @@ -88,7 +94,7 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection } try { // Create and run the outgoing session - SyncSession out = createDuplexOutgoingSession(ctx, writer); + SyncSession out = createDuplexOutgoingSession(ctx, writer, null); setOutgoingSession(out); out.run(); writer.dispose(false); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java index 9bf5980f8..2644d8cf4 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java @@ -6,6 +6,7 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportConnectionReader; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.properties.TransportPropertyManager; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.SyncSessionFactory; import org.briarproject.bramble.api.transport.KeyManager; import org.briarproject.bramble.api.transport.StreamContext; @@ -60,8 +61,11 @@ class IncomingSimplexSyncConnection extends SyncConnection implements Runnable { } connectionRegistry.registerConnection(contactId, transportId, true); try { + // We don't expect to receive a priority for this connection + PriorityHandler handler = p -> + LOG.info("Ignoring priority for simplex connection"); // Create and run the incoming session - createIncomingSession(ctx, reader).run(); + createIncomingSession(ctx, reader, handler).run(); reader.dispose(false, true); } catch (IOException e) { logException(LOG, WARNING, e); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java index ed6d21d05..0da1c4a18 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java @@ -7,6 +7,8 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.duplex.DuplexTransportConnection; import org.briarproject.bramble.api.properties.TransportPropertyManager; +import org.briarproject.bramble.api.sync.Priority; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.SyncSession; import org.briarproject.bramble.api.sync.SyncSessionFactory; import org.briarproject.bramble.api.transport.KeyManager; @@ -15,15 +17,18 @@ import org.briarproject.bramble.api.transport.StreamReaderFactory; import org.briarproject.bramble.api.transport.StreamWriterFactory; import java.io.IOException; +import java.security.SecureRandom; import java.util.concurrent.Executor; import static java.util.logging.Level.WARNING; +import static org.briarproject.bramble.api.sync.SyncConstants.PRIORITY_NONCE_BYTES; import static org.briarproject.bramble.util.LogUtils.logException; @NotNullByDefault class OutgoingDuplexSyncConnection extends DuplexSyncConnection implements Runnable { + private final SecureRandom secureRandom; private final ContactId contactId; OutgoingDuplexSyncConnection(KeyManager keyManager, @@ -32,11 +37,14 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, ContactId contactId, TransportId transportId, - DuplexTransportConnection connection) { + Executor ioExecutor, ConnectionChooser connectionChooser, + SecureRandom secureRandom, ContactId contactId, + TransportId transportId, DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, - transportPropertyManager, ioExecutor, transportId, connection); + transportPropertyManager, ioExecutor, connectionChooser, + transportId, connection); + this.secureRandom = secureRandom; this.contactId = contactId; } @@ -56,10 +64,12 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection return; } // Start the incoming session on another thread - ioExecutor.execute(this::runIncomingSession); + Priority priority = generatePriority(); + ioExecutor.execute(() -> runIncomingSession(priority)); try { // Create and run the outgoing session - SyncSession out = createDuplexOutgoingSession(ctx, writer); + SyncSession out = + createDuplexOutgoingSession(ctx, writer, priority); setOutgoingSession(out); out.run(); writer.dispose(false); @@ -69,7 +79,7 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection } } - private void runIncomingSession() { + private void runIncomingSession(Priority priority) { // Read and recognise the tag StreamContext ctx = recogniseTag(reader, transportId); // Unrecognised tags are suspicious in this case @@ -97,12 +107,16 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection return; } connectionRegistry.registerConnection(contactId, transportId, false); + connectionChooser.addConnection(contactId, transportId, this, priority); try { // Store any transport properties discovered from the connection transportPropertyManager.addRemotePropertiesFromConnection( contactId, transportId, remote); + // We don't expect to receive a priority for this connection + PriorityHandler handler = p -> + LOG.info("Ignoring priority for outgoing connection"); // Create and run the incoming session - createIncomingSession(ctx, reader).run(); + createIncomingSession(ctx, reader, handler).run(); reader.dispose(false, true); interruptOutgoingSession(); } catch (DbException | IOException e) { @@ -111,6 +125,7 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection } finally { connectionRegistry.unregisterConnection(contactId, transportId, false); + connectionChooser.removeConnection(contactId, transportId, this); } } @@ -118,4 +133,10 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection // 'Recognised' is always true for outgoing connections onReadError(true); } + + private Priority generatePriority() { + byte[] nonce = new byte[PRIORITY_NONCE_BYTES]; + secureRandom.nextBytes(nonce); + return new Priority(nonce); + } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/SyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/SyncConnection.java index 40ac2cac2..6de535552 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/SyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/SyncConnection.java @@ -7,6 +7,7 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportConnectionReader; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.properties.TransportPropertyManager; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.SyncSession; import org.briarproject.bramble.api.sync.SyncSessionFactory; import org.briarproject.bramble.api.transport.KeyManager; @@ -52,10 +53,12 @@ class SyncConnection extends Connection { } SyncSession createIncomingSession(StreamContext ctx, - TransportConnectionReader r) throws IOException { + TransportConnectionReader r, PriorityHandler handler) + throws IOException { InputStream streamReader = streamReaderFactory.createStreamReader( r.getInputStream(), ctx); ContactId c = requireNonNull(ctx.getContactId()); - return syncSessionFactory.createIncomingSession(c, streamReader); + return syncSessionFactory + .createIncomingSession(c, streamReader, handler); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/sync/DuplexOutgoingSession.java b/bramble-core/src/main/java/org/briarproject/bramble/sync/DuplexOutgoingSession.java index 0aa1a8985..fc007cd0f 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/sync/DuplexOutgoingSession.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/sync/DuplexOutgoingSession.java @@ -14,6 +14,7 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.sync.Ack; import org.briarproject.bramble.api.sync.Message; import org.briarproject.bramble.api.sync.Offer; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.api.sync.Request; import org.briarproject.bramble.api.sync.SyncRecordWriter; import org.briarproject.bramble.api.sync.SyncSession; @@ -35,6 +36,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Logger; +import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -74,6 +76,8 @@ class DuplexOutgoingSession implements SyncSession, EventListener { private final int maxLatency, maxIdleTime; private final StreamWriter streamWriter; private final SyncRecordWriter recordWriter; + @Nullable + private final Priority priority; private final BlockingQueue> writerTasks; private final AtomicBoolean generateAckQueued = new AtomicBoolean(false); @@ -88,7 +92,7 @@ class DuplexOutgoingSession implements SyncSession, EventListener { DuplexOutgoingSession(DatabaseComponent db, Executor dbExecutor, EventBus eventBus, Clock clock, ContactId contactId, int maxLatency, int maxIdleTime, StreamWriter streamWriter, - SyncRecordWriter recordWriter) { + SyncRecordWriter recordWriter, @Nullable Priority priority) { this.db = db; this.dbExecutor = dbExecutor; this.eventBus = eventBus; @@ -98,6 +102,7 @@ class DuplexOutgoingSession implements SyncSession, EventListener { this.maxIdleTime = maxIdleTime; this.streamWriter = streamWriter; this.recordWriter = recordWriter; + this.priority = priority; writerTasks = new LinkedBlockingQueue<>(); } @@ -108,6 +113,8 @@ class DuplexOutgoingSession implements SyncSession, EventListener { try { // Send our supported protocol versions recordWriter.writeVersions(new Versions(SUPPORTED_VERSIONS)); + // Send our connection priority, if this is an outgoing connection + if (priority != null) recordWriter.writePriority(priority); // Start a query for each type of record generateAck(); generateBatch(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/sync/IncomingSession.java b/bramble-core/src/main/java/org/briarproject/bramble/sync/IncomingSession.java index e9c17577e..6414b0490 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/sync/IncomingSession.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/sync/IncomingSession.java @@ -15,6 +15,8 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.sync.Ack; import org.briarproject.bramble.api.sync.Message; import org.briarproject.bramble.api.sync.Offer; +import org.briarproject.bramble.api.sync.Priority; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.Request; import org.briarproject.bramble.api.sync.SyncRecordReader; import org.briarproject.bramble.api.sync.SyncSession; @@ -47,17 +49,19 @@ class IncomingSession implements SyncSession, EventListener { private final EventBus eventBus; private final ContactId contactId; private final SyncRecordReader recordReader; + private final PriorityHandler priorityHandler; private volatile boolean interrupted = false; IncomingSession(DatabaseComponent db, Executor dbExecutor, EventBus eventBus, ContactId contactId, - SyncRecordReader recordReader) { + SyncRecordReader recordReader, PriorityHandler priorityHandler) { this.db = db; this.dbExecutor = dbExecutor; this.eventBus = eventBus; this.contactId = contactId; this.recordReader = recordReader; + this.priorityHandler = priorityHandler; } @IoExecutor @@ -86,6 +90,9 @@ class IncomingSession implements SyncSession, EventListener { } else if (recordReader.hasVersions()) { Versions v = recordReader.readVersions(); dbExecutor.execute(new ReceiveVersions(v)); + } else if (recordReader.hasPriority()) { + Priority p = recordReader.readPriority(); + priorityHandler.handle(p); } else { // unknown records are ignored in RecordReader#eof() throw new FormatException(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncSessionFactoryImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncSessionFactoryImpl.java index d35e1164b..64bf25c5d 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncSessionFactoryImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/sync/SyncSessionFactoryImpl.java @@ -5,6 +5,8 @@ import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DatabaseExecutor; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.sync.Priority; +import org.briarproject.bramble.api.sync.PriorityHandler; import org.briarproject.bramble.api.sync.SyncRecordReader; import org.briarproject.bramble.api.sync.SyncRecordReaderFactory; import org.briarproject.bramble.api.sync.SyncRecordWriter; @@ -18,6 +20,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.concurrent.Executor; +import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import javax.inject.Inject; @@ -46,10 +49,12 @@ class SyncSessionFactoryImpl implements SyncSessionFactory { } @Override - public SyncSession createIncomingSession(ContactId c, InputStream in) { + public SyncSession createIncomingSession(ContactId c, InputStream in, + PriorityHandler handler) { SyncRecordReader recordReader = recordReaderFactory.createRecordReader(in); - return new IncomingSession(db, dbExecutor, eventBus, c, recordReader); + return new IncomingSession(db, dbExecutor, eventBus, c, recordReader, + handler); } @Override @@ -64,11 +69,12 @@ class SyncSessionFactoryImpl implements SyncSessionFactory { @Override public SyncSession createDuplexOutgoingSession(ContactId c, int maxLatency, - int maxIdleTime, StreamWriter streamWriter) { + int maxIdleTime, StreamWriter streamWriter, + @Nullable Priority priority) { OutputStream out = streamWriter.getOutputStream(); SyncRecordWriter recordWriter = recordWriterFactory.createRecordWriter(out); return new DuplexOutgoingSession(db, dbExecutor, eventBus, clock, c, - maxLatency, maxIdleTime, streamWriter, recordWriter); + maxLatency, maxIdleTime, streamWriter, recordWriter, priority); } } From 8dd993dd9d7219331906432997845ad8643efae4 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 13 May 2020 10:24:27 +0100 Subject: [PATCH 04/21] Interrupt connections outside the lock. --- .../bramble/connection/ConnectionChooserImpl.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java index e3c64e449..0832611c2 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java @@ -32,6 +32,7 @@ class ConnectionChooserImpl implements ConnectionChooser { @Override public void addConnection(ContactId c, TransportId t, DuplexSyncConnection conn, Priority p) { + DuplexSyncConnection close = null; synchronized (lock) { Key k = new Key(c, t); Value best = bestConnections.get(k); @@ -39,12 +40,14 @@ class ConnectionChooserImpl implements ConnectionChooser { bestConnections.put(k, new Value(conn, p)); } else if (compare(p.getNonce(), best.priority.getNonce()) > 0) { LOG.info("Found a better connection"); + close = best.connection; bestConnections.put(k, new Value(conn, p)); - best.connection.interruptOutgoingSession(); + } else { + LOG.info("Already have a better connection"); + close = conn; } - LOG.info("Already have a better connection"); - conn.interruptOutgoingSession(); } + if (close != null) close.interruptOutgoingSession(); } @Override From 0c338b362e96d8d70fdf1ac4be8e0373bd6ae247 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 13 May 2020 15:35:03 +0100 Subject: [PATCH 05/21] Add InterruptibleConnection interface for easier testing. --- .../bramble/connection/ConnectionChooser.java | 14 ++++++++++++-- .../connection/ConnectionChooserImpl.java | 10 +++++----- .../connection/DuplexSyncConnection.java | 6 ++++-- .../connection/InterruptibleConnection.java | 19 +++++++++++++++++++ 4 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java index da26c904f..300339878 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java @@ -5,18 +5,28 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.sync.Priority; +/** + * Chooses one connection per contact and transport to keep open and closes + * any other connections. + */ @NotNullByDefault interface ConnectionChooser { /** * Adds the given connection to the chooser with the given priority. + *

+ * If the chooser has a connection with the same contact and transport and + * a lower {@link Priority priority}, that connection will be + * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. + * If the chooser has a connection with the same contact and transport and + * a higher priority, the newly added connection will be interrupted. */ - void addConnection(ContactId c, TransportId t, DuplexSyncConnection conn, + void addConnection(ContactId c, TransportId t, InterruptibleConnection conn, Priority p); /** * Removes the given connection from the chooser. */ void removeConnection(ContactId c, TransportId t, - DuplexSyncConnection conn); + InterruptibleConnection conn); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java index 0832611c2..b73f74fed 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java @@ -31,8 +31,8 @@ class ConnectionChooserImpl implements ConnectionChooser { @Override public void addConnection(ContactId c, TransportId t, - DuplexSyncConnection conn, Priority p) { - DuplexSyncConnection close = null; + InterruptibleConnection conn, Priority p) { + InterruptibleConnection close = null; synchronized (lock) { Key k = new Key(c, t); Value best = bestConnections.get(k); @@ -52,7 +52,7 @@ class ConnectionChooserImpl implements ConnectionChooser { @Override public void removeConnection(ContactId c, TransportId t, - DuplexSyncConnection conn) { + InterruptibleConnection conn) { synchronized (lock) { Key k = new Key(c, t); Value best = bestConnections.get(k); @@ -89,10 +89,10 @@ class ConnectionChooserImpl implements ConnectionChooser { private static class Value { - private final DuplexSyncConnection connection; + private final InterruptibleConnection connection; private final Priority priority; - private Value(DuplexSyncConnection connection, Priority priority) { + private Value(InterruptibleConnection connection, Priority priority) { this.connection = connection; this.priority = priority; } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java index 2f5415d26..778e7b4d8 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java @@ -27,7 +27,8 @@ import javax.annotation.concurrent.GuardedBy; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; @NotNullByDefault -abstract class DuplexSyncConnection extends SyncConnection { +abstract class DuplexSyncConnection extends SyncConnection + implements InterruptibleConnection { final Executor ioExecutor; final ConnectionChooser connectionChooser; @@ -44,7 +45,8 @@ abstract class DuplexSyncConnection extends SyncConnection { @GuardedBy("interruptLock") private boolean interruptWaiting = false; - void interruptOutgoingSession() { + @Override + public void interruptOutgoingSession() { synchronized (interruptLock) { if (outgoingSession == null) interruptWaiting = true; else outgoingSession.interrupt(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java new file mode 100644 index 000000000..e9924964d --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java @@ -0,0 +1,19 @@ +package org.briarproject.bramble.connection; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +/** + * A duplex sync connection that can be closed by interrupting its outgoing + * sync session. + */ +@NotNullByDefault +interface InterruptibleConnection { + + /** + * Interrupts the connection's outgoing sync session. If the underlying + * transport connection is alive and the remote peer is cooperative, this + * should result in both sync sessions ending and the connection being + * cleanly closed. + */ + void interruptOutgoingSession(); +} From 2919657b4a0c2b5395a81e9e64c60933465464e4 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 13 May 2020 15:56:07 +0100 Subject: [PATCH 06/21] Add unit tests for connection chooser. --- .../connection/ConnectionChooserImplTest.java | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java diff --git a/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java new file mode 100644 index 000000000..82ddc02ab --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java @@ -0,0 +1,80 @@ +package org.briarproject.bramble.connection; + +import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.plugin.TransportId; +import org.briarproject.bramble.api.sync.Priority; +import org.briarproject.bramble.test.BrambleMockTestCase; +import org.jmock.Expectations; +import org.junit.Before; +import org.junit.Test; + +import static org.briarproject.bramble.api.sync.SyncConstants.PRIORITY_NONCE_BYTES; +import static org.briarproject.bramble.test.TestUtils.getContactId; +import static org.briarproject.bramble.test.TestUtils.getRandomBytes; +import static org.briarproject.bramble.test.TestUtils.getTransportId; +import static org.briarproject.bramble.util.StringUtils.fromHexString; + +public class ConnectionChooserImplTest extends BrambleMockTestCase { + + private final InterruptibleConnection conn1 = + context.mock(InterruptibleConnection.class, "conn1"); + private final InterruptibleConnection conn2 = + context.mock(InterruptibleConnection.class, "conn2"); + + private final ContactId contactId = getContactId(); + private final TransportId transportId = getTransportId(); + + private final Priority low = + new Priority(fromHexString("00000000000000000000000000000000")); + private final Priority high = + new Priority(fromHexString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF")); + + private ConnectionChooserImpl chooser; + + @Before + public void setUp() { + chooser = new ConnectionChooserImpl(); + } + + @Test + public void testOldConnectionIsInterruptedIfNewHasHigherPriority() { + chooser.addConnection(contactId, transportId, conn1, low); + + context.checking(new Expectations() {{ + oneOf(conn1).interruptOutgoingSession(); + }}); + + chooser.addConnection(contactId, transportId, conn2, high); + } + + @Test + public void testNewConnectionIsInterruptedIfOldHasHigherPriority() { + chooser.addConnection(contactId, transportId, conn1, high); + + context.checking(new Expectations() {{ + oneOf(conn2).interruptOutgoingSession(); + }}); + + chooser.addConnection(contactId, transportId, conn2, low); + } + + @Test + public void testConnectionIsNotInterruptedAfterBeingRemoved() { + chooser.addConnection(contactId, transportId, conn1, low); + chooser.removeConnection(contactId, transportId, conn1); + chooser.addConnection(contactId, transportId, conn2, high); + } + + @Test + public void testConnectionIsInterruptedIfAddedTwice() { + chooser.addConnection(contactId, transportId, conn1, + new Priority(getRandomBytes(PRIORITY_NONCE_BYTES))); + + context.checking(new Expectations() {{ + oneOf(conn1).interruptOutgoingSession(); + }}); + + chooser.addConnection(contactId, transportId, conn1, + new Priority(getRandomBytes(PRIORITY_NONCE_BYTES))); + } +} From d3d7212b084d23be01430d9b556f79e10ad221fd Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 13 May 2020 17:55:05 +0100 Subject: [PATCH 07/21] Add registry method for deciding which contacts to poll. --- .../api/connection/ConnectionRegistry.java | 6 ++++ .../bramble/api/plugin/PluginConfig.java | 8 +++++ .../connection/ConnectionRegistryImpl.java | 30 +++++++++++++++++-- .../ConnectionRegistryImplTest.java | 19 ++++++++++-- .../bramble/test/TestPluginConfigModule.java | 9 ++++++ .../bramble/plugin/DesktopPluginModule.java | 13 ++++++++ .../briarproject/briar/android/AppModule.java | 14 +++++++++ .../briar/headless/HeadlessModule.kt | 7 +++-- .../briar/headless/HeadlessTestModule.kt | 7 +++++ 9 files changed, 106 insertions(+), 7 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java index 715a4da9e..6afc2ee7e 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java @@ -40,6 +40,12 @@ public interface ConnectionRegistry { */ Collection getConnectedContacts(TransportId t); + /** + * Returns any contacts that are connected via the given transport, or via + * any transport that's preferred to the given transport. + */ + Collection getConnectedOrPreferredContacts(TransportId t); + /** * Returns true if the given contact is connected via the given transport. */ diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java index 6bf13cedd..4da41d6d2 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java @@ -1,10 +1,12 @@ package org.briarproject.bramble.api.plugin; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory; import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import java.util.Collection; +import java.util.List; @NotNullByDefault public interface PluginConfig { @@ -14,4 +16,10 @@ public interface PluginConfig { Collection getSimplexFactories(); boolean shouldPoll(); + + /** + * Returns a list of transport preferences. For each pair in the list, + * the first transport is preferred to the second. + */ + List> getTransportPreferences(); } 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 1bba22593..0f580a4cd 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 @@ -1,11 +1,13 @@ package org.briarproject.bramble.connection; import org.briarproject.bramble.api.Multiset; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.plugin.PluginConfig; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.event.ConnectionClosedEvent; import org.briarproject.bramble.api.plugin.event.ConnectionOpenedEvent; @@ -16,7 +18,6 @@ import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedE import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -28,6 +29,7 @@ import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; +import static java.util.Collections.emptyList; import static java.util.logging.Level.INFO; import static java.util.logging.Logger.getLogger; @@ -39,6 +41,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { getLogger(ConnectionRegistryImpl.class.getName()); private final EventBus eventBus; + private final List> preferences; private final Object lock = new Object(); @GuardedBy("lock") @@ -49,8 +52,9 @@ class ConnectionRegistryImpl implements ConnectionRegistry { private final Set connectedPendingContacts; @Inject - ConnectionRegistryImpl(EventBus eventBus) { + ConnectionRegistryImpl(EventBus eventBus, PluginConfig pluginConfig) { this.eventBus = eventBus; + preferences = pluginConfig.getTransportPreferences(); contactConnections = new HashMap<>(); contactCounts = new Multiset<>(); connectedPendingContacts = new HashSet<>(); @@ -106,7 +110,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { public Collection getConnectedContacts(TransportId t) { synchronized (lock) { Multiset m = contactConnections.get(t); - if (m == null) return Collections.emptyList(); + if (m == null) return emptyList(); List ids = new ArrayList<>(m.keySet()); if (LOG.isLoggable(INFO)) LOG.info(ids.size() + " contacts connected: " + t); @@ -114,6 +118,26 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } } + @Override + public Collection getConnectedOrPreferredContacts( + TransportId t) { + synchronized (lock) { + Multiset m = contactConnections.get(t); + if (m == null) return emptyList(); + Set ids = new HashSet<>(m.keySet()); + for (Pair pair : preferences) { + if (pair.getSecond().equals(t)) { + TransportId better = pair.getFirst(); + Multiset m1 = contactConnections.get(better); + if (m1 != null) ids.addAll(m1.keySet()); + } + } + if (LOG.isLoggable(INFO)) + LOG.info(ids.size() + " contacts connected or preferred: " + t); + return ids; + } + } + @Override public boolean isConnected(ContactId c, TransportId t) { synchronized (lock) { 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 3edf4ddc8..51da97ae2 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 @@ -4,6 +4,7 @@ import org.briarproject.bramble.api.connection.ConnectionRegistry; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.event.EventBus; +import org.briarproject.bramble.api.plugin.PluginConfig; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.event.ConnectionClosedEvent; import org.briarproject.bramble.api.plugin.event.ConnectionOpenedEvent; @@ -18,6 +19,7 @@ import org.junit.Test; import java.util.Collection; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.briarproject.bramble.test.TestUtils.getContactId; import static org.briarproject.bramble.test.TestUtils.getRandomId; @@ -30,6 +32,7 @@ import static org.junit.Assert.fail; public class ConnectionRegistryImplTest extends BrambleMockTestCase { private final EventBus eventBus = context.mock(EventBus.class); + private final PluginConfig pluginConfig = context.mock(PluginConfig.class); private final ContactId contactId = getContactId(); private final ContactId contactId1 = getContactId(); @@ -40,7 +43,13 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { @Test public void testRegisterAndUnregister() { - ConnectionRegistry c = new ConnectionRegistryImpl(eventBus); + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue(emptyMap())); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); // The registry should be empty assertEquals(emptyList(), c.getConnectedContacts(transportId)); @@ -122,7 +131,13 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { @Test public void testRegisterAndUnregisterPendingContacts() { - ConnectionRegistry c = new ConnectionRegistryImpl(eventBus); + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue(emptyMap())); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any( diff --git a/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java b/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java index 689d4c5bd..f081d887f 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java @@ -1,5 +1,6 @@ package org.briarproject.bramble.test; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.PluginCallback; import org.briarproject.bramble.api.plugin.PluginConfig; @@ -10,12 +11,14 @@ import org.briarproject.bramble.api.plugin.simplex.SimplexPlugin; import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import java.util.Collection; +import java.util.List; import javax.annotation.Nullable; import dagger.Module; import dagger.Provides; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.briarproject.bramble.test.TestUtils.getTransportId; @@ -85,6 +88,12 @@ public class TestPluginConfigModule { public boolean shouldPoll() { return false; } + + + @Override + public List> getTransportPreferences() { + return emptyList(); + } }; return pluginConfig; } diff --git a/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java b/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java index c00bd99d7..05e28499d 100644 --- a/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java +++ b/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java @@ -1,12 +1,16 @@ package org.briarproject.bramble.plugin; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.io.TimeoutMonitor; import org.briarproject.bramble.api.lifecycle.IoExecutor; import org.briarproject.bramble.api.lifecycle.ShutdownManager; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.BackoffFactory; +import org.briarproject.bramble.api.plugin.BluetoothConstants; +import org.briarproject.bramble.api.plugin.LanTcpConstants; import org.briarproject.bramble.api.plugin.PluginConfig; +import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory; import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import org.briarproject.bramble.api.reliability.ReliabilityLayerFactory; @@ -17,6 +21,7 @@ import org.briarproject.bramble.plugin.tcp.WanTcpPluginFactory; import java.security.SecureRandom; import java.util.Collection; +import java.util.List; import java.util.concurrent.Executor; import dagger.Module; @@ -24,6 +29,7 @@ import dagger.Provides; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; @Module public class DesktopPluginModule extends PluginModule { @@ -61,6 +67,13 @@ public class DesktopPluginModule extends PluginModule { public boolean shouldPoll() { return true; } + + @Override + public List> getTransportPreferences() { + // Prefer LAN to Bluetooth + return singletonList( + new Pair<>(LanTcpConstants.ID, BluetoothConstants.ID)); + } }; return pluginConfig; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java index b9e074897..d2dace83c 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java @@ -8,6 +8,7 @@ import android.os.StrictMode; import com.vanniktech.emoji.RecentEmoji; import org.briarproject.bramble.api.FeatureFlags; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.battery.BatteryManager; import org.briarproject.bramble.api.crypto.CryptoComponent; import org.briarproject.bramble.api.crypto.KeyStrengthener; @@ -20,7 +21,10 @@ import org.briarproject.bramble.api.lifecycle.LifecycleManager; import org.briarproject.bramble.api.network.NetworkManager; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.BackoffFactory; +import org.briarproject.bramble.api.plugin.BluetoothConstants; +import org.briarproject.bramble.api.plugin.LanTcpConstants; import org.briarproject.bramble.api.plugin.PluginConfig; +import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory; import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import org.briarproject.bramble.api.reporting.DevConfig; @@ -48,6 +52,7 @@ import java.io.File; import java.security.GeneralSecurityException; import java.security.SecureRandom; import java.util.Collection; +import java.util.List; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; @@ -62,6 +67,7 @@ import static android.content.Context.MODE_PRIVATE; import static android.os.Build.VERSION.SDK_INT; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_ONION_ADDRESS; import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_PUBLIC_KEY_HEX; import static org.briarproject.briar.android.TestingConstants.IS_DEBUG_BUILD; @@ -153,6 +159,14 @@ public class AppModule { public boolean shouldPoll() { return true; } + + + @Override + public List> getTransportPreferences() { + // Prefer LAN to Bluetooth + return singletonList( + new Pair<>(LanTcpConstants.ID, BluetoothConstants.ID)); + } }; return pluginConfig; } diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt index 5f622c83b..f1b354192 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt @@ -4,13 +4,13 @@ import com.fasterxml.jackson.databind.ObjectMapper import dagger.Module import dagger.Provides import org.briarproject.bramble.api.FeatureFlags +import org.briarproject.bramble.api.Pair import org.briarproject.bramble.api.battery.BatteryManager import org.briarproject.bramble.api.db.DatabaseConfig import org.briarproject.bramble.api.event.EventBus import org.briarproject.bramble.api.lifecycle.IoExecutor import org.briarproject.bramble.api.network.NetworkManager -import org.briarproject.bramble.api.plugin.BackoffFactory -import org.briarproject.bramble.api.plugin.PluginConfig +import org.briarproject.bramble.api.plugin.* import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory import org.briarproject.bramble.api.system.Clock @@ -33,6 +33,7 @@ import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File import java.util.Collections.emptyList +import java.util.Collections.singletonList import java.util.concurrent.Executor import javax.inject.Singleton import javax.net.SocketFactory @@ -88,6 +89,8 @@ internal class HeadlessModule(private val appDir: File) { override fun getDuplexFactories(): Collection = duplex override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = true + override fun getTransportPreferences(): List> = + singletonList(Pair(LanTcpConstants.ID, BluetoothConstants.ID)) } } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt index e0e70fac2..4d4bee12c 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt @@ -4,8 +4,12 @@ import com.fasterxml.jackson.databind.ObjectMapper import dagger.Module import dagger.Provides import org.briarproject.bramble.api.FeatureFlags +import org.briarproject.bramble.api.Pair import org.briarproject.bramble.api.db.DatabaseConfig +import org.briarproject.bramble.api.plugin.BluetoothConstants +import org.briarproject.bramble.api.plugin.LanTcpConstants import org.briarproject.bramble.api.plugin.PluginConfig +import org.briarproject.bramble.api.plugin.TransportId import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory import org.briarproject.bramble.network.JavaNetworkModule @@ -19,6 +23,7 @@ import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File import java.util.Collections.emptyList +import java.util.Collections.singletonList import javax.inject.Singleton @Module( @@ -55,6 +60,8 @@ internal class HeadlessTestModule(private val appDir: File) { override fun getDuplexFactories(): Collection = emptyList() override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = false + override fun getTransportPreferences(): List> = + singletonList(Pair(LanTcpConstants.ID, BluetoothConstants.ID)) } } From e8dbc007123562881e3777dd950a8fe0710dcc67 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 14:22:27 +0100 Subject: [PATCH 08/21] Refactor connection registry implementation. --- .../connection/ConnectionRegistryImpl.java | 121 ++++++++++++------ .../ConnectionRegistryImplTest.java | 5 +- 2 files changed, 86 insertions(+), 40 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 0f580a4cd..3563c4850 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 @@ -1,6 +1,5 @@ package org.briarproject.bramble.connection; -import org.briarproject.bramble.api.Multiset; import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; import org.briarproject.bramble.api.contact.ContactId; @@ -22,6 +21,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.logging.Logger; @@ -41,22 +41,30 @@ class ConnectionRegistryImpl implements ConnectionRegistry { getLogger(ConnectionRegistryImpl.class.getName()); private final EventBus eventBus; - private final List> preferences; + private final Map> betterTransports; private final Object lock = new Object(); @GuardedBy("lock") - private final Map> contactConnections; - @GuardedBy("lock") - private final Multiset contactCounts; + private final Map> contactConnections; @GuardedBy("lock") private final Set connectedPendingContacts; @Inject ConnectionRegistryImpl(EventBus eventBus, PluginConfig pluginConfig) { this.eventBus = eventBus; - preferences = pluginConfig.getTransportPreferences(); + betterTransports = new HashMap<>(); + for (Pair pair : + pluginConfig.getTransportPreferences()) { + TransportId better = pair.getFirst(); + TransportId worse = pair.getSecond(); + List list = betterTransports.get(worse); + if (list == null) { + list = new ArrayList<>(); + betterTransports.put(worse, list); + } + list.add(better); + } contactConnections = new HashMap<>(); - contactCounts = new Multiset<>(); connectedPendingContacts = new HashSet<>(); } @@ -69,13 +77,13 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } boolean firstConnection = false; synchronized (lock) { - Multiset m = contactConnections.get(t); - if (m == null) { - m = new Multiset<>(); - contactConnections.put(t, m); + List recs = contactConnections.get(c); + if (recs == null) { + recs = new ArrayList<>(); + contactConnections.put(c, recs); } - m.add(c); - if (contactCounts.add(c) == 1) firstConnection = true; + if (recs.isEmpty()) firstConnection = true; + recs.add(new ConnectionRecord(t)); } eventBus.broadcast(new ConnectionOpenedEvent(c, t, incoming)); if (firstConnection) { @@ -93,11 +101,10 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } boolean lastConnection = false; synchronized (lock) { - Multiset m = contactConnections.get(t); - if (m == null || !m.contains(c)) + List recs = contactConnections.get(c); + if (recs == null || !recs.remove(new ConnectionRecord(t))) throw new IllegalArgumentException(); - m.remove(c); - if (contactCounts.remove(c) == 0) lastConnection = true; + if (recs.isEmpty()) lastConnection = true; } eventBus.broadcast(new ConnectionClosedEvent(c, t, incoming)); if (lastConnection) { @@ -109,12 +116,20 @@ class ConnectionRegistryImpl implements ConnectionRegistry { @Override public Collection getConnectedContacts(TransportId t) { synchronized (lock) { - Multiset m = contactConnections.get(t); - if (m == null) return emptyList(); - List ids = new ArrayList<>(m.keySet()); - if (LOG.isLoggable(INFO)) - LOG.info(ids.size() + " contacts connected: " + t); - return ids; + List contactIds = new ArrayList<>(); + for (Entry> e : + contactConnections.entrySet()) { + for (ConnectionRecord rec : e.getValue()) { + if (rec.transportId.equals(t)) { + contactIds.add(e.getKey()); + break; + } + } + } + if (LOG.isLoggable(INFO)) { + LOG.info(contactIds.size() + " contacts connected: " + t); + } + return contactIds; } } @@ -122,34 +137,43 @@ class ConnectionRegistryImpl implements ConnectionRegistry { public Collection getConnectedOrPreferredContacts( TransportId t) { synchronized (lock) { - Multiset m = contactConnections.get(t); - if (m == null) return emptyList(); - Set ids = new HashSet<>(m.keySet()); - for (Pair pair : preferences) { - if (pair.getSecond().equals(t)) { - TransportId better = pair.getFirst(); - Multiset m1 = contactConnections.get(better); - if (m1 != null) ids.addAll(m1.keySet()); + List better = betterTransports.get(t); + if (better == null) better = emptyList(); + List contactIds = new ArrayList<>(); + for (Entry> e : + contactConnections.entrySet()) { + for (ConnectionRecord rec : e.getValue()) { + if (rec.transportId.equals(t) || + better.contains(rec.transportId)) { + contactIds.add(e.getKey()); + break; + } } } - if (LOG.isLoggable(INFO)) - LOG.info(ids.size() + " contacts connected or preferred: " + t); - return ids; + if (LOG.isLoggable(INFO)) { + LOG.info(contactIds.size() + + " contacts connected or preferred: " + t); + } + return contactIds; } } @Override public boolean isConnected(ContactId c, TransportId t) { synchronized (lock) { - Multiset m = contactConnections.get(t); - return m != null && m.contains(c); + List recs = contactConnections.get(c); + if (recs == null) return false; + for (ConnectionRecord rec : recs) { + if (rec.transportId.equals(t)) return true; + } + return false; } } @Override public boolean isConnected(ContactId c) { synchronized (lock) { - return contactCounts.contains(c); + return contactConnections.containsKey(c); } } @@ -171,4 +195,27 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } eventBus.broadcast(new RendezvousConnectionClosedEvent(p, success)); } + + private static class ConnectionRecord { + + private final TransportId transportId; + + private ConnectionRecord(TransportId transportId) { + this.transportId = transportId; + } + + @Override + public boolean equals(Object o) { + if (o instanceof ConnectionRecord) { + ConnectionRecord rec = (ConnectionRecord) o; + return transportId.equals(rec.transportId); + } + return false; + } + + @Override + public int hashCode() { + return transportId.hashCode(); + } + } } 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 51da97ae2..3b1dcf208 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 @@ -19,7 +19,6 @@ import org.junit.Test; import java.util.Collection; import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.briarproject.bramble.test.TestUtils.getContactId; import static org.briarproject.bramble.test.TestUtils.getRandomId; @@ -45,7 +44,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testRegisterAndUnregister() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyMap())); + will(returnValue(emptyList())); }}); ConnectionRegistry c = @@ -133,7 +132,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testRegisterAndUnregisterPendingContacts() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyMap())); + will(returnValue(emptyList())); }}); ConnectionRegistry c = From 36747acac14c089e8318b28baa93c33c39bcd2ae Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 14:47:34 +0100 Subject: [PATCH 09/21] Extract better and worse transports from preferences. --- .../connection/ConnectionRegistryImpl.java | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 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 3563c4850..9aa92d646 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 @@ -42,6 +42,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { private final EventBus eventBus; private final Map> betterTransports; + private final Map> worseTransports; private final Object lock = new Object(); @GuardedBy("lock") @@ -53,21 +54,32 @@ class ConnectionRegistryImpl implements ConnectionRegistry { ConnectionRegistryImpl(EventBus eventBus, PluginConfig pluginConfig) { this.eventBus = eventBus; betterTransports = new HashMap<>(); - for (Pair pair : - pluginConfig.getTransportPreferences()) { - TransportId better = pair.getFirst(); - TransportId worse = pair.getSecond(); - List list = betterTransports.get(worse); - if (list == null) { - list = new ArrayList<>(); - betterTransports.put(worse, list); - } - list.add(better); - } + worseTransports = new HashMap<>(); + initTransportPreferences(pluginConfig.getTransportPreferences()); contactConnections = new HashMap<>(); connectedPendingContacts = new HashSet<>(); } + private void initTransportPreferences( + List> prefs) { + for (Pair pair : prefs) { + TransportId better = pair.getFirst(); + TransportId worse = pair.getSecond(); + List betterList = betterTransports.get(worse); + if (betterList == null) { + betterList = new ArrayList<>(); + betterTransports.put(worse, betterList); + } + betterList.add(better); + List worseList = worseTransports.get(better); + if (worseList == null) { + worseList = new ArrayList<>(); + worseTransports.put(better, worseList); + } + worseList.add(worse); + } + } + @Override public void registerConnection(ContactId c, TransportId t, boolean incoming) { From 7d6b65913a7e761f3141f3300b80eff9b7578c82 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 16:42:01 +0100 Subject: [PATCH 10/21] Combine connection chooser with connection registry. --- .../api/connection/ConnectionRegistry.java | 39 +- .../connection/InterruptibleConnection.java | 4 +- .../bramble/connection/ConnectionChooser.java | 32 -- .../connection/ConnectionChooserImpl.java | 100 ----- .../connection/ConnectionManagerImpl.java | 8 +- .../bramble/connection/ConnectionModule.java | 7 - .../connection/ConnectionRegistryImpl.java | 126 +++++- .../connection/DuplexSyncConnection.java | 7 +- .../IncomingDuplexSyncConnection.java | 17 +- .../IncomingSimplexSyncConnection.java | 4 - .../OutgoingDuplexSyncConnection.java | 14 +- .../OutgoingSimplexSyncConnection.java | 4 - .../connection/ConnectionChooserImplTest.java | 80 ---- .../ConnectionRegistryImplTest.java | 413 ++++++++++++++++-- 14 files changed, 542 insertions(+), 313 deletions(-) rename {bramble-core/src/main/java/org/briarproject/bramble => bramble-api/src/main/java/org/briarproject/bramble/api}/connection/InterruptibleConnection.java (83%) delete mode 100644 bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java delete mode 100644 bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java delete mode 100644 bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java index 6afc2ee7e..1b552101e 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java @@ -3,6 +3,7 @@ package org.briarproject.bramble.api.connection; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.plugin.PluginConfig; import org.briarproject.bramble.api.plugin.TransportId; import org.briarproject.bramble.api.plugin.event.ConnectionClosedEvent; import org.briarproject.bramble.api.plugin.event.ConnectionOpenedEvent; @@ -10,6 +11,7 @@ import org.briarproject.bramble.api.plugin.event.ContactConnectedEvent; import org.briarproject.bramble.api.plugin.event.ContactDisconnectedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionClosedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedEvent; +import org.briarproject.bramble.api.sync.Priority; import java.util.Collection; @@ -21,19 +23,46 @@ public interface ConnectionRegistry { /** * Registers a connection with the given contact over the given transport. + *

+ * If the registry has any connections with the same contact and a + * {@link PluginConfig#getTransportPreferences() worse} transport, those + * connections will be + * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. + *

+ * If the registry has any connections with the same contact and a better + * transport, the given connection will be interrupted. + *

* Broadcasts {@link ConnectionOpenedEvent}. Also broadcasts * {@link ContactConnectedEvent} if this is the only connection with the * contact. */ - void registerConnection(ContactId c, TransportId t, boolean incoming); + void registerConnection(ContactId c, TransportId t, + InterruptibleConnection conn, boolean incoming); /** * Unregisters a connection with the given contact over the given transport. + *

* Broadcasts {@link ConnectionClosedEvent}. Also broadcasts * {@link ContactDisconnectedEvent} if this is the only connection with * the contact. */ - void unregisterConnection(ContactId c, TransportId t, boolean incoming); + void unregisterConnection(ContactId c, TransportId t, + InterruptibleConnection conn, boolean incoming); + + /** + * Sets the {@link Priority priority} of a connection that was previously + * registered via {@link #registerConnection(ContactId, TransportId, + * InterruptibleConnection, boolean)}. + *

+ * If the registry has any connections with the same contact and transport + * and a lower {@link Priority priority}, those connections will be + * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. + *

+ * If the registry has any connections with the same contact and transport + * and a higher priority, the given connection will be interrupted. + */ + void setPriority(ContactId c, TransportId t, InterruptibleConnection conn, + Priority priority); /** * Returns any contacts that are connected via the given transport. @@ -41,10 +70,10 @@ public interface ConnectionRegistry { Collection getConnectedContacts(TransportId t); /** - * Returns any contacts that are connected via the given transport, or via - * any transport that's preferred to the given transport. + * Returns any contacts that are connected via the given transport or any + * {@link PluginConfig#getTransportPreferences() better} transport. */ - Collection getConnectedOrPreferredContacts(TransportId t); + Collection getConnectedOrBetterContacts(TransportId t); /** * Returns true if the given contact is connected via the given transport. diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/InterruptibleConnection.java similarity index 83% rename from bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java rename to bramble-api/src/main/java/org/briarproject/bramble/api/connection/InterruptibleConnection.java index e9924964d..59b46b1ec 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/InterruptibleConnection.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/InterruptibleConnection.java @@ -1,4 +1,4 @@ -package org.briarproject.bramble.connection; +package org.briarproject.bramble.api.connection; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -7,7 +7,7 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; * sync session. */ @NotNullByDefault -interface InterruptibleConnection { +public interface InterruptibleConnection { /** * Interrupts the connection's outgoing sync session. If the underlying diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java deleted file mode 100644 index 300339878..000000000 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooser.java +++ /dev/null @@ -1,32 +0,0 @@ -package org.briarproject.bramble.connection; - -import org.briarproject.bramble.api.contact.ContactId; -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; -import org.briarproject.bramble.api.plugin.TransportId; -import org.briarproject.bramble.api.sync.Priority; - -/** - * Chooses one connection per contact and transport to keep open and closes - * any other connections. - */ -@NotNullByDefault -interface ConnectionChooser { - - /** - * Adds the given connection to the chooser with the given priority. - *

- * If the chooser has a connection with the same contact and transport and - * a lower {@link Priority priority}, that connection will be - * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. - * If the chooser has a connection with the same contact and transport and - * a higher priority, the newly added connection will be interrupted. - */ - void addConnection(ContactId c, TransportId t, InterruptibleConnection conn, - Priority p); - - /** - * Removes the given connection from the chooser. - */ - void removeConnection(ContactId c, TransportId t, - InterruptibleConnection conn); -} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java deleted file mode 100644 index b73f74fed..000000000 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionChooserImpl.java +++ /dev/null @@ -1,100 +0,0 @@ -package org.briarproject.bramble.connection; - -import org.briarproject.bramble.api.contact.ContactId; -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; -import org.briarproject.bramble.api.plugin.TransportId; -import org.briarproject.bramble.api.sync.Priority; - -import java.util.HashMap; -import java.util.Map; -import java.util.logging.Logger; - -import javax.annotation.concurrent.GuardedBy; -import javax.inject.Inject; - -import static java.util.logging.Logger.getLogger; -import static org.briarproject.bramble.api.Bytes.compare; - -@NotNullByDefault -class ConnectionChooserImpl implements ConnectionChooser { - - private static final Logger LOG = - getLogger(ConnectionChooserImpl.class.getName()); - - private final Object lock = new Object(); - @GuardedBy("lock") - private final Map bestConnections = new HashMap<>(); - - @Inject - ConnectionChooserImpl() { - } - - @Override - public void addConnection(ContactId c, TransportId t, - InterruptibleConnection conn, Priority p) { - InterruptibleConnection close = null; - synchronized (lock) { - Key k = new Key(c, t); - Value best = bestConnections.get(k); - if (best == null) { - bestConnections.put(k, new Value(conn, p)); - } else if (compare(p.getNonce(), best.priority.getNonce()) > 0) { - LOG.info("Found a better connection"); - close = best.connection; - bestConnections.put(k, new Value(conn, p)); - } else { - LOG.info("Already have a better connection"); - close = conn; - } - } - if (close != null) close.interruptOutgoingSession(); - } - - @Override - public void removeConnection(ContactId c, TransportId t, - InterruptibleConnection conn) { - synchronized (lock) { - Key k = new Key(c, t); - Value best = bestConnections.get(k); - if (best.connection == conn) bestConnections.remove(k); - } - } - - private static class Key { - - private final ContactId contactId; - private final TransportId transportId; - - private Key(ContactId contactId, TransportId transportId) { - this.contactId = contactId; - this.transportId = transportId; - } - - @Override - public int hashCode() { - return contactId.hashCode(); - } - - @Override - public boolean equals(Object o) { - if (o instanceof Key) { - Key k = (Key) o; - return contactId.equals(k.contactId) && - transportId.equals(k.transportId); - } else { - return false; - } - } - } - - private static class Value { - - private final InterruptibleConnection connection; - private final Priority priority; - - private Value(InterruptibleConnection connection, Priority priority) { - this.connection = connection; - this.priority = priority; - } - } -} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java index 71914cb3c..2a50033b1 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionManagerImpl.java @@ -37,7 +37,6 @@ class ConnectionManagerImpl implements ConnectionManager { private final ContactExchangeManager contactExchangeManager; private final ConnectionRegistry connectionRegistry; private final TransportPropertyManager transportPropertyManager; - private final ConnectionChooser connectionChooser; private final SecureRandom secureRandom; @Inject @@ -49,7 +48,7 @@ class ConnectionManagerImpl implements ConnectionManager { ContactExchangeManager contactExchangeManager, ConnectionRegistry connectionRegistry, TransportPropertyManager transportPropertyManager, - ConnectionChooser connectionChooser, SecureRandom secureRandom) { + SecureRandom secureRandom) { this.ioExecutor = ioExecutor; this.keyManager = keyManager; this.streamReaderFactory = streamReaderFactory; @@ -59,7 +58,6 @@ class ConnectionManagerImpl implements ConnectionManager { this.contactExchangeManager = contactExchangeManager; this.connectionRegistry = connectionRegistry; this.transportPropertyManager = transportPropertyManager; - this.connectionChooser = connectionChooser; this.secureRandom = secureRandom; } @@ -78,7 +76,7 @@ class ConnectionManagerImpl implements ConnectionManager { ioExecutor.execute(new IncomingDuplexSyncConnection(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager, ioExecutor, - connectionChooser, t, d)); + t, d)); } @Override @@ -103,7 +101,7 @@ class ConnectionManagerImpl implements ConnectionManager { ioExecutor.execute(new OutgoingDuplexSyncConnection(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager, ioExecutor, - connectionChooser, secureRandom, c, t, d)); + secureRandom, c, t, d)); } @Override diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java index 08f299352..e6b8fee3a 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/ConnectionModule.java @@ -23,11 +23,4 @@ public class ConnectionModule { ConnectionRegistryImpl connectionRegistry) { return connectionRegistry; } - - @Provides - @Singleton - ConnectionChooser provideConnectionChooser( - ConnectionChooserImpl connectionChooser) { - return connectionChooser; - } } 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 9aa92d646..b7337c7c0 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 @@ -1,7 +1,9 @@ package org.briarproject.bramble.connection; +import org.briarproject.bramble.api.Bytes; import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; +import org.briarproject.bramble.api.connection.InterruptibleConnection; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.event.EventBus; @@ -14,6 +16,7 @@ import org.briarproject.bramble.api.plugin.event.ContactConnectedEvent; import org.briarproject.bramble.api.plugin.event.ContactDisconnectedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionClosedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedEvent; +import org.briarproject.bramble.api.sync.Priority; import java.util.ArrayList; import java.util.Collection; @@ -25,6 +28,7 @@ import java.util.Map.Entry; import java.util.Set; import java.util.logging.Logger; +import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; @@ -42,7 +46,6 @@ class ConnectionRegistryImpl implements ConnectionRegistry { private final EventBus eventBus; private final Map> betterTransports; - private final Map> worseTransports; private final Object lock = new Object(); @GuardedBy("lock") @@ -54,7 +57,6 @@ class ConnectionRegistryImpl implements ConnectionRegistry { ConnectionRegistryImpl(EventBus eventBus, PluginConfig pluginConfig) { this.eventBus = eventBus; betterTransports = new HashMap<>(); - worseTransports = new HashMap<>(); initTransportPreferences(pluginConfig.getTransportPreferences()); contactConnections = new HashMap<>(); connectedPendingContacts = new HashSet<>(); @@ -71,31 +73,50 @@ class ConnectionRegistryImpl implements ConnectionRegistry { betterTransports.put(worse, betterList); } betterList.add(better); - List worseList = worseTransports.get(better); - if (worseList == null) { - worseList = new ArrayList<>(); - worseTransports.put(better, worseList); - } - worseList.add(worse); } } @Override public void registerConnection(ContactId c, TransportId t, - boolean incoming) { + InterruptibleConnection conn, boolean incoming) { if (LOG.isLoggable(INFO)) { if (incoming) LOG.info("Incoming connection registered: " + t); else LOG.info("Outgoing connection registered: " + t); } - boolean firstConnection = false; + List toInterrupt; + boolean firstConnection = false, interruptNewConnection = false; synchronized (lock) { List recs = contactConnections.get(c); if (recs == null) { recs = new ArrayList<>(); contactConnections.put(c, recs); } - if (recs.isEmpty()) firstConnection = true; - recs.add(new ConnectionRecord(t)); + 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; + } + } + } + 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) { @@ -104,9 +125,61 @@ 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) { + if (LOG.isLoggable(INFO)) LOG.info("Setting connection priority: " + t); + List toInterrupt; + boolean interruptNewConnection = false; + synchronized (lock) { + List recs = contactConnections.get(c); + if (recs == null) throw new IllegalArgumentException(); + toInterrupt = new ArrayList<>(recs.size()); + for (ConnectionRecord rec : recs) { + 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); + 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; + } + } + } + } + if (interruptNewConnection) { + LOG.info("Interrupting new connection"); + conn.interruptOutgoingSession(); + } + for (InterruptibleConnection old : toInterrupt) { + LOG.info("Interrupting old connection"); + old.interruptOutgoingSession(); + } + } + + private int compare(Priority a, @Nullable Priority b) { + return b == null ? 0 : Bytes.compare(a.getNonce(), b.getNonce()); + } + @Override public void unregisterConnection(ContactId c, TransportId t, - boolean incoming) { + InterruptibleConnection conn, boolean incoming) { if (LOG.isLoggable(INFO)) { if (incoming) LOG.info("Incoming connection unregistered: " + t); else LOG.info("Outgoing connection unregistered: " + t); @@ -114,7 +187,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { boolean lastConnection = false; synchronized (lock) { List recs = contactConnections.get(c); - if (recs == null || !recs.remove(new ConnectionRecord(t))) + if (recs == null || !recs.remove(new ConnectionRecord(t, conn))) throw new IllegalArgumentException(); if (recs.isEmpty()) lastConnection = true; } @@ -146,8 +219,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } @Override - public Collection getConnectedOrPreferredContacts( - TransportId t) { + public Collection getConnectedOrBetterContacts(TransportId t) { synchronized (lock) { List better = betterTransports.get(t); if (better == null) better = emptyList(); @@ -164,7 +236,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } if (LOG.isLoggable(INFO)) { LOG.info(contactIds.size() - + " contacts connected or preferred: " + t); + + " contacts connected or better: " + t); } return contactIds; } @@ -208,26 +280,34 @@ class ConnectionRegistryImpl implements ConnectionRegistry { eventBus.broadcast(new RendezvousConnectionClosedEvent(p, success)); } - private static class ConnectionRecord { + private class ConnectionRecord { private final TransportId transportId; + private final InterruptibleConnection conn; + @GuardedBy("lock") + @Nullable + private Priority priority = null; + @GuardedBy("lock") + private boolean interrupted = false; - private ConnectionRecord(TransportId transportId) { + private ConnectionRecord(TransportId transportId, + InterruptibleConnection conn) { this.transportId = transportId; + this.conn = conn; } @Override public boolean equals(Object o) { if (o instanceof ConnectionRecord) { - ConnectionRecord rec = (ConnectionRecord) o; - return transportId.equals(rec.transportId); + return conn == ((ConnectionRecord) o).conn; + } else { + return false; } - return false; } @Override public int hashCode() { - return transportId.hashCode(); + return conn.hashCode(); } } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java index 778e7b4d8..6053b10c9 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/DuplexSyncConnection.java @@ -1,6 +1,7 @@ package org.briarproject.bramble.connection; import org.briarproject.bramble.api.connection.ConnectionRegistry; +import org.briarproject.bramble.api.connection.InterruptibleConnection; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.TransportConnectionReader; @@ -31,7 +32,6 @@ abstract class DuplexSyncConnection extends SyncConnection implements InterruptibleConnection { final Executor ioExecutor; - final ConnectionChooser connectionChooser; final TransportId transportId; final TransportConnectionReader reader; final TransportConnectionWriter writer; @@ -69,13 +69,12 @@ abstract class DuplexSyncConnection extends SyncConnection StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, ConnectionChooser connectionChooser, - TransportId transportId, DuplexTransportConnection connection) { + Executor ioExecutor, TransportId transportId, + DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, transportPropertyManager); this.ioExecutor = ioExecutor; - this.connectionChooser = connectionChooser; this.transportId = transportId; reader = connection.getReader(); writer = connection.getWriter(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java index 5b72532e2..f1b53e768 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java @@ -31,12 +31,11 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, ConnectionChooser connectionChooser, - TransportId transportId, DuplexTransportConnection connection) { + Executor ioExecutor, TransportId transportId, + DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, - transportPropertyManager, ioExecutor, connectionChooser, - transportId, connection); + transportPropertyManager, ioExecutor, transportId, connection); } @Override @@ -60,15 +59,16 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection onReadError(true); return; } - connectionRegistry.registerConnection(contactId, transportId, true); + connectionRegistry.registerConnection(contactId, transportId, + this, true); // Start the outgoing session on another thread ioExecutor.execute(() -> runOutgoingSession(contactId)); try { // Store any transport properties discovered from the connection transportPropertyManager.addRemotePropertiesFromConnection( contactId, transportId, remote); - // Add the connection to the chooser when we receive its priority - PriorityHandler handler = p -> connectionChooser.addConnection( + // Update the connection registry when we receive our priority + PriorityHandler handler = p -> connectionRegistry.setPriority( contactId, transportId, this, p); // Create and run the incoming session createIncomingSession(ctx, reader, handler).run(); @@ -79,8 +79,7 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection onReadError(true); } finally { connectionRegistry.unregisterConnection(contactId, transportId, - true); - connectionChooser.removeConnection(contactId, transportId, this); + this, true); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java index 2644d8cf4..b41fb33e0 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingSimplexSyncConnection.java @@ -59,7 +59,6 @@ class IncomingSimplexSyncConnection extends SyncConnection implements Runnable { onError(true); return; } - connectionRegistry.registerConnection(contactId, transportId, true); try { // We don't expect to receive a priority for this connection PriorityHandler handler = p -> @@ -70,9 +69,6 @@ class IncomingSimplexSyncConnection extends SyncConnection implements Runnable { } catch (IOException e) { logException(LOG, WARNING, e); onError(true); - } finally { - connectionRegistry.unregisterConnection(contactId, transportId, - true); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java index 0da1c4a18..a4d237071 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java @@ -37,13 +37,11 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection StreamWriterFactory streamWriterFactory, SyncSessionFactory syncSessionFactory, TransportPropertyManager transportPropertyManager, - Executor ioExecutor, ConnectionChooser connectionChooser, - SecureRandom secureRandom, ContactId contactId, + Executor ioExecutor, SecureRandom secureRandom, ContactId contactId, TransportId transportId, DuplexTransportConnection connection) { super(keyManager, connectionRegistry, streamReaderFactory, streamWriterFactory, syncSessionFactory, - transportPropertyManager, ioExecutor, connectionChooser, - transportId, connection); + transportPropertyManager, ioExecutor, transportId, connection); this.secureRandom = secureRandom; this.contactId = contactId; } @@ -106,8 +104,9 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection onReadError(); return; } - connectionRegistry.registerConnection(contactId, transportId, false); - connectionChooser.addConnection(contactId, transportId, this, priority); + connectionRegistry.registerConnection(contactId, transportId, + this, false); + connectionRegistry.setPriority(contactId, transportId, this, priority); try { // Store any transport properties discovered from the connection transportPropertyManager.addRemotePropertiesFromConnection( @@ -124,8 +123,7 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection onReadError(); } finally { connectionRegistry.unregisterConnection(contactId, transportId, - false); - connectionChooser.removeConnection(contactId, transportId, this); + this, false); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingSimplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingSimplexSyncConnection.java index 7136c818f..d91d444c2 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingSimplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingSimplexSyncConnection.java @@ -52,7 +52,6 @@ class OutgoingSimplexSyncConnection extends SyncConnection implements Runnable { onError(); return; } - connectionRegistry.registerConnection(contactId, transportId, false); try { // Create and run the outgoing session createSimplexOutgoingSession(ctx, writer).run(); @@ -60,9 +59,6 @@ class OutgoingSimplexSyncConnection extends SyncConnection implements Runnable { } catch (IOException e) { logException(LOG, WARNING, e); onError(); - } finally { - connectionRegistry.unregisterConnection(contactId, transportId, - false); } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java deleted file mode 100644 index 82ddc02ab..000000000 --- a/bramble-core/src/test/java/org/briarproject/bramble/connection/ConnectionChooserImplTest.java +++ /dev/null @@ -1,80 +0,0 @@ -package org.briarproject.bramble.connection; - -import org.briarproject.bramble.api.contact.ContactId; -import org.briarproject.bramble.api.plugin.TransportId; -import org.briarproject.bramble.api.sync.Priority; -import org.briarproject.bramble.test.BrambleMockTestCase; -import org.jmock.Expectations; -import org.junit.Before; -import org.junit.Test; - -import static org.briarproject.bramble.api.sync.SyncConstants.PRIORITY_NONCE_BYTES; -import static org.briarproject.bramble.test.TestUtils.getContactId; -import static org.briarproject.bramble.test.TestUtils.getRandomBytes; -import static org.briarproject.bramble.test.TestUtils.getTransportId; -import static org.briarproject.bramble.util.StringUtils.fromHexString; - -public class ConnectionChooserImplTest extends BrambleMockTestCase { - - private final InterruptibleConnection conn1 = - context.mock(InterruptibleConnection.class, "conn1"); - private final InterruptibleConnection conn2 = - context.mock(InterruptibleConnection.class, "conn2"); - - private final ContactId contactId = getContactId(); - private final TransportId transportId = getTransportId(); - - private final Priority low = - new Priority(fromHexString("00000000000000000000000000000000")); - private final Priority high = - new Priority(fromHexString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF")); - - private ConnectionChooserImpl chooser; - - @Before - public void setUp() { - chooser = new ConnectionChooserImpl(); - } - - @Test - public void testOldConnectionIsInterruptedIfNewHasHigherPriority() { - chooser.addConnection(contactId, transportId, conn1, low); - - context.checking(new Expectations() {{ - oneOf(conn1).interruptOutgoingSession(); - }}); - - chooser.addConnection(contactId, transportId, conn2, high); - } - - @Test - public void testNewConnectionIsInterruptedIfOldHasHigherPriority() { - chooser.addConnection(contactId, transportId, conn1, high); - - context.checking(new Expectations() {{ - oneOf(conn2).interruptOutgoingSession(); - }}); - - chooser.addConnection(contactId, transportId, conn2, low); - } - - @Test - public void testConnectionIsNotInterruptedAfterBeingRemoved() { - chooser.addConnection(contactId, transportId, conn1, low); - chooser.removeConnection(contactId, transportId, conn1); - chooser.addConnection(contactId, transportId, conn2, high); - } - - @Test - public void testConnectionIsInterruptedIfAddedTwice() { - chooser.addConnection(contactId, transportId, conn1, - new Priority(getRandomBytes(PRIORITY_NONCE_BYTES))); - - context.checking(new Expectations() {{ - oneOf(conn1).interruptOutgoingSession(); - }}); - - chooser.addConnection(contactId, transportId, conn1, - new Priority(getRandomBytes(PRIORITY_NONCE_BYTES))); - } -} 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 3b1dcf208..5c361c3d6 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 @@ -1,6 +1,8 @@ package org.briarproject.bramble.connection; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; +import org.briarproject.bramble.api.connection.InterruptibleConnection; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.event.EventBus; @@ -12,6 +14,7 @@ import org.briarproject.bramble.api.plugin.event.ContactConnectedEvent; import org.briarproject.bramble.api.plugin.event.ContactDisconnectedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionClosedEvent; import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedEvent; +import org.briarproject.bramble.api.sync.Priority; import org.briarproject.bramble.test.BrambleMockTestCase; import org.jmock.Expectations; import org.junit.Test; @@ -23,6 +26,7 @@ import static java.util.Collections.singletonList; import static org.briarproject.bramble.test.TestUtils.getContactId; import static org.briarproject.bramble.test.TestUtils.getRandomId; import static org.briarproject.bramble.test.TestUtils.getTransportId; +import static org.briarproject.bramble.util.StringUtils.fromHexString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -32,16 +36,28 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { private final EventBus eventBus = context.mock(EventBus.class); private final PluginConfig pluginConfig = context.mock(PluginConfig.class); + private final InterruptibleConnection conn1 = + context.mock(InterruptibleConnection.class, "conn1"); + private final InterruptibleConnection conn2 = + context.mock(InterruptibleConnection.class, "conn2"); + private final InterruptibleConnection conn3 = + context.mock(InterruptibleConnection.class, "conn3"); - private final ContactId contactId = getContactId(); private final ContactId contactId1 = getContactId(); - private final TransportId transportId = getTransportId(); + private final ContactId contactId2 = getContactId(); private final TransportId transportId1 = getTransportId(); + private final TransportId transportId2 = getTransportId(); + private final TransportId transportId3 = getTransportId(); private final PendingContactId pendingContactId = new PendingContactId(getRandomId()); + private final Priority low = + new Priority(fromHexString("00000000000000000000000000000000")); + private final Priority high = + new Priority(fromHexString("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF")); + @Test - public void testRegisterAndUnregister() { + public void testRegisterMultipleConnections() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); will(returnValue(emptyList())); @@ -51,8 +67,12 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { new ConnectionRegistryImpl(eventBus, pluginConfig); // The registry should be empty - assertEquals(emptyList(), c.getConnectedContacts(transportId)); assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId1)); + assertEquals(emptyList(), c.getConnectedContacts(transportId2)); + assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId2)); + assertEquals(emptyList(), c.getConnectedContacts(transportId3)); + assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId3)); // Check that a registered connection shows up - this should // broadcast a ConnectionOpenedEvent and a ContactConnectedEvent @@ -60,34 +80,41 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); oneOf(eventBus).broadcast(with(any(ContactConnectedEvent.class))); }}); - c.registerConnection(contactId, transportId, true); - assertEquals(singletonList(contactId), - c.getConnectedContacts(transportId)); - assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + c.registerConnection(contactId1, transportId1, conn1, true); context.assertIsSatisfied(); - // Register an identical connection - this should broadcast a - // ConnectionOpenedEvent and lookup should be unaffected + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Register another connection with the same contact and transport - + // this should broadcast a ConnectionOpenedEvent and lookup should be + // unaffected context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId, transportId, true); - assertEquals(singletonList(contactId), - c.getConnectedContacts(transportId)); - assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + c.registerConnection(contactId1, transportId1, conn2, true); context.assertIsSatisfied(); + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + // Unregister one of the connections - this should broadcast a // ConnectionClosedEvent and lookup should be unaffected context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); }}); - c.unregisterConnection(contactId, transportId, true); - assertEquals(singletonList(contactId), - c.getConnectedContacts(transportId)); - assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + c.unregisterConnection(contactId1, transportId1, conn1, true); context.assertIsSatisfied(); + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + // Unregister the other connection - this should broadcast a // ConnectionClosedEvent and a ContactDisconnectedEvent context.checking(new Expectations() {{ @@ -95,37 +122,363 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(eventBus).broadcast(with(any( ContactDisconnectedEvent.class))); }}); - c.unregisterConnection(contactId, transportId, true); - assertEquals(emptyList(), c.getConnectedContacts(transportId)); - assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + c.unregisterConnection(contactId1, transportId1, conn2, true); context.assertIsSatisfied(); + assertEquals(emptyList(), c.getConnectedContacts(transportId1)); + assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId1)); + // Try to unregister the connection again - exception should be thrown try { - c.unregisterConnection(contactId, transportId, true); + c.unregisterConnection(contactId1, transportId1, conn2, true); fail(); } catch (IllegalArgumentException expected) { // Expected } + } - // Register both contacts with one transport, one contact with both - - // this should broadcast three ConnectionOpenedEvents and two - // ContactConnectedEvents + @Test + public void testRegisterMultipleContacts() { + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue(emptyList())); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); + + // Register two contacts with one transport, then one of the contacts + // with a second transport - this should broadcast three + // ConnectionOpenedEvents and two ContactConnectedEvents context.checking(new Expectations() {{ exactly(3).of(eventBus).broadcast(with(any( ConnectionOpenedEvent.class))); exactly(2).of(eventBus).broadcast(with(any( ContactConnectedEvent.class))); }}); - c.registerConnection(contactId, transportId, true); - c.registerConnection(contactId1, transportId, true); - c.registerConnection(contactId1, transportId1, true); - Collection connected = c.getConnectedContacts(transportId); + c.registerConnection(contactId1, transportId1, conn1, true); + c.registerConnection(contactId2, transportId1, conn2, true); + c.registerConnection(contactId2, transportId2, conn3, true); + context.assertIsSatisfied(); + + Collection connected = c.getConnectedContacts(transportId1); assertEquals(2, connected.size()); - assertTrue(connected.contains(contactId)); assertTrue(connected.contains(contactId1)); + assertTrue(connected.contains(contactId2)); + + connected = c.getConnectedOrBetterContacts(transportId1); + assertEquals(2, connected.size()); + assertTrue(connected.contains(contactId1)); + assertTrue(connected.contains(contactId2)); + + assertEquals(singletonList(contactId2), + c.getConnectedContacts(transportId2)); + assertEquals(singletonList(contactId2), + c.getConnectedOrBetterContacts(transportId2)); + } + + @Test + public void testNewConnectionIsInterruptedIfOldConnectionUsesBetterTransport() { + // Prefer transport 1 to transport 2 + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue( + singletonList(new Pair<>(transportId1, transportId2)))); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); + + // Connect via transport 1 (better than 2) + 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)); + + // The contact is not connected via transport 2 but is connected via a + // better transport + assertEquals(emptyList(), c.getConnectedContacts(transportId2)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId2)); + + // Connect via transport 2 (worse than 1) - the new connection should + // be interrupted + context.checking(new Expectations() {{ + oneOf(conn2).interruptOutgoingSession(); + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId2, conn2, true); + 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) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId3, conn3, true); + 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)); + + // Unregister the interrupted connection (transport 2) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); + }}); + c.unregisterConnection(contactId1, transportId2, conn2, true); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // The contact is not connected via transport 2 but is connected via a + // better transport + assertEquals(emptyList(), c.getConnectedContacts(transportId2)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId2)); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId3)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId3)); + } + + @Test + public void testOldConnectionIsInterruptedIfNewConnectionUsesBetterTransport() { + // 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) + 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) - the old connection should + // be interrupted + context.checking(new Expectations() {{ + oneOf(conn1).interruptOutgoingSession(); + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId2, conn2, true); + 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) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId3, conn3, true); + 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)); + + // Unregister the interrupted connection (transport 1) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); + }}); + c.unregisterConnection(contactId1, transportId1, conn1, true); + context.assertIsSatisfied(); + + // The contact is not connected via transport 1 but is connected via a + // better transport + assertEquals(emptyList(), 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 testNewConnectionIsInterruptedIfOldConnectionHasHigherPriority() { + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue(emptyList())); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); + + // Register a connection with high priority + 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), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Register another connection via the same transport (no priority yet) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId1, conn2, true); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Set the priority of the second connection to low - the second + // connection should be interrupted + context.checking(new Expectations() {{ + oneOf(conn2).interruptOutgoingSession(); + }}); + c.setPriority(contactId1, transportId1, conn2, low); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Register a third connection with low priority - it should also be + // interrupted + context.checking(new Expectations() {{ + oneOf(conn3).interruptOutgoingSession(); + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId1, conn3, true); + c.setPriority(contactId1, transportId1, conn3, low); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + } + + @Test + public void testOldConnectionIsInterruptedIfNewConnectionHasHigherPriority() { + context.checking(new Expectations() {{ + allowing(pluginConfig).getTransportPreferences(); + will(returnValue(emptyList())); + }}); + + ConnectionRegistry c = + new ConnectionRegistryImpl(eventBus, pluginConfig); + + // Register a connection with low priority + 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), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Register another connection via the same transport (no priority yet) + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); + }}); + c.registerConnection(contactId1, transportId1, conn2, true); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); + + // Set the priority of the second connection to high - the first + // connection should be interrupted + context.checking(new Expectations() {{ + oneOf(conn1).interruptOutgoingSession(); + }}); + c.setPriority(contactId1, transportId1, conn2, high); + context.assertIsSatisfied(); + + assertEquals(singletonList(contactId1), + c.getConnectedContacts(transportId1)); + assertEquals(singletonList(contactId1), + c.getConnectedOrBetterContacts(transportId1)); } @Test From 5b04527c54df4a21fb4b0e377e0d2f072765bac0 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 17:47:16 +0100 Subject: [PATCH 11/21] Fix screenshot test. --- .../java/org/briarproject/briar/android/SetupDataTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java b/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java index ee47bf86f..a5771f379 100644 --- a/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java +++ b/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java @@ -120,7 +120,8 @@ public class SetupDataTest extends ScreenshotTest { // TODO add messages - connectionRegistry.registerConnection(bob.getId(), ID, true); + connectionRegistry.registerConnection(bob.getId(), ID, () -> { + }, true); } } From 4aaa8c3b9368d0d1bfddecefefbfc21e764411c7 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 17:16:42 +0100 Subject: [PATCH 12/21] Don't poll if already connected via a better transport. --- .../main/java/org/briarproject/bramble/plugin/PollerImpl.java | 2 +- .../java/org/briarproject/bramble/plugin/PollerImplTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java index c42e49a8c..a0b72dc55 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java @@ -215,7 +215,7 @@ class PollerImpl implements Poller, EventListener { Map remote = transportPropertyManager.getRemoteProperties(t); Collection connected = - connectionRegistry.getConnectedContacts(t); + connectionRegistry.getConnectedOrBetterContacts(t); Collection> properties = new ArrayList<>(); for (Entry e : remote.entrySet()) { diff --git a/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java index 522c9aad9..9d56b2ef4 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java @@ -354,7 +354,7 @@ public class PollerImplTest extends BrambleMockTestCase { // Get the transport properties and connected contacts oneOf(transportPropertyManager).getRemoteProperties(transportId); will(returnValue(singletonMap(contactId, properties))); - oneOf(connectionRegistry).getConnectedContacts(transportId); + oneOf(connectionRegistry).getConnectedOrBetterContacts(transportId); will(returnValue(emptyList())); // Poll the plugin oneOf(plugin).poll(with(collectionOf( @@ -397,7 +397,7 @@ public class PollerImplTest extends BrambleMockTestCase { // Get the transport properties and connected contacts oneOf(transportPropertyManager).getRemoteProperties(transportId); will(returnValue(singletonMap(contactId, properties))); - oneOf(connectionRegistry).getConnectedContacts(transportId); + oneOf(connectionRegistry).getConnectedOrBetterContacts(transportId); will(returnValue(singletonList(contactId))); // All contacts are connected, so don't poll the plugin }}); From d3751fbeadd47b1559cdddae5b56bdb638a0f3ba Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 1 Jun 2020 14:23:08 +0100 Subject: [PATCH 13/21] 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))); }}); From 2add63657e10389c7f78adfef56f63e4f5ad289f Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 1 Jun 2020 14:35:38 +0100 Subject: [PATCH 14/21] Inner class can be static. --- .../briarproject/bramble/connection/ConnectionRegistryImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 192423b1c..e88bdf5b4 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 @@ -253,7 +253,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { eventBus.broadcast(new RendezvousConnectionClosedEvent(p, success)); } - private class ConnectionRecord { + private static class ConnectionRecord { private final TransportId transportId; private final InterruptibleConnection conn; From 35d1b406f75803bc2756e441dae4a59409fc54c8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 1 Jun 2020 14:44:21 +0100 Subject: [PATCH 15/21] Refactor transport preferences. --- .../bramble/api/plugin/PluginConfig.java | 9 ++++---- .../connection/ConnectionRegistryImpl.java | 22 +++---------------- .../ConnectionRegistryImplTest.java | 19 ++++++++-------- .../bramble/test/TestPluginConfigModule.java | 10 ++++----- .../bramble/plugin/DesktopPluginModule.java | 9 ++++---- .../briarproject/briar/android/AppModule.java | 10 ++++----- .../briar/headless/HeadlessModule.kt | 8 +++---- .../briar/headless/HeadlessTestModule.kt | 8 +++---- 8 files changed, 41 insertions(+), 54 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java index 4da41d6d2..61063030c 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/PluginConfig.java @@ -1,12 +1,12 @@ package org.briarproject.bramble.api.plugin; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory; import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import java.util.Collection; import java.util.List; +import java.util.Map; @NotNullByDefault public interface PluginConfig { @@ -18,8 +18,9 @@ public interface PluginConfig { boolean shouldPoll(); /** - * Returns a list of transport preferences. For each pair in the list, - * the first transport is preferred to the second. + * Returns a map representing transport preferences. For each entry in the + * map, connections via the transports identified by the value are + * preferred to connections via the transport identified by the key. */ - List> getTransportPreferences(); + Map> getTransportPreferences(); } 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 e88bdf5b4..d735a7c2f 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 @@ -1,7 +1,6 @@ package org.briarproject.bramble.connection; import org.briarproject.bramble.api.Bytes; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; import org.briarproject.bramble.api.connection.InterruptibleConnection; import org.briarproject.bramble.api.contact.ContactId; @@ -45,7 +44,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { getLogger(ConnectionRegistryImpl.class.getName()); private final EventBus eventBus; - private final Map> betterTransports; + private final Map> transportPrefs; private final Object lock = new Object(); @GuardedBy("lock") @@ -56,26 +55,11 @@ class ConnectionRegistryImpl implements ConnectionRegistry { @Inject ConnectionRegistryImpl(EventBus eventBus, PluginConfig pluginConfig) { this.eventBus = eventBus; - betterTransports = new HashMap<>(); - initTransportPreferences(pluginConfig.getTransportPreferences()); + transportPrefs = pluginConfig.getTransportPreferences(); contactConnections = new HashMap<>(); connectedPendingContacts = new HashSet<>(); } - private void initTransportPreferences( - List> prefs) { - for (Pair pair : prefs) { - TransportId better = pair.getFirst(); - TransportId worse = pair.getSecond(); - List betterList = betterTransports.get(worse); - if (betterList == null) { - betterList = new ArrayList<>(); - betterTransports.put(worse, betterList); - } - betterList.add(better); - } - } - @Override public void registerConnection(ContactId c, TransportId t, InterruptibleConnection conn, boolean incoming) { @@ -146,7 +130,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } private List getBetterTransports(TransportId t) { - List better = betterTransports.get(t); + List better = transportPrefs.get(t); return better == null ? emptyList() : better; } 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 cb63a9fc7..1a1fbc6b1 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 @@ -1,6 +1,5 @@ package org.briarproject.bramble.connection; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.connection.ConnectionRegistry; import org.briarproject.bramble.api.connection.InterruptibleConnection; import org.briarproject.bramble.api.contact.ContactId; @@ -22,7 +21,9 @@ import org.junit.Test; import java.util.Collection; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.briarproject.bramble.test.TestUtils.getContactId; import static org.briarproject.bramble.test.TestUtils.getRandomId; import static org.briarproject.bramble.test.TestUtils.getTransportId; @@ -60,7 +61,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testRegisterMultipleConnections() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyList())); + will(returnValue(emptyMap())); }}); ConnectionRegistry c = @@ -141,7 +142,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testRegisterMultipleContacts() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyList())); + will(returnValue(emptyMap())); }}); ConnectionRegistry c = @@ -183,7 +184,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); will(returnValue( - singletonList(new Pair<>(transportId2, transportId1)))); + singletonMap(transportId1, singletonList(transportId2)))); }}); ConnectionRegistry c = @@ -256,7 +257,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); will(returnValue( - singletonList(new Pair<>(transportId1, transportId2)))); + singletonMap(transportId2, singletonList(transportId1)))); }}); ConnectionRegistry c = @@ -357,7 +358,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); will(returnValue( - singletonList(new Pair<>(transportId2, transportId1)))); + singletonMap(transportId1, singletonList(transportId2)))); }}); ConnectionRegistry c = @@ -452,7 +453,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testNewConnectionIsInterruptedIfOldConnectionHasHigherPriority() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyList())); + will(returnValue(emptyMap())); }}); ConnectionRegistry c = @@ -517,7 +518,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testOldConnectionIsInterruptedIfNewConnectionHasHigherPriority() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyList())); + will(returnValue(emptyMap())); }}); ConnectionRegistry c = @@ -567,7 +568,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { public void testRegisterAndUnregisterPendingContacts() { context.checking(new Expectations() {{ allowing(pluginConfig).getTransportPreferences(); - will(returnValue(emptyList())); + will(returnValue(emptyMap())); }}); ConnectionRegistry c = diff --git a/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java b/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java index f081d887f..4b3347884 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/test/TestPluginConfigModule.java @@ -1,6 +1,5 @@ package org.briarproject.bramble.test; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.plugin.PluginCallback; import org.briarproject.bramble.api.plugin.PluginConfig; @@ -12,13 +11,14 @@ import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory; import java.util.Collection; import java.util.List; +import java.util.Map; import javax.annotation.Nullable; import dagger.Module; import dagger.Provides; -import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.briarproject.bramble.test.TestUtils.getTransportId; @@ -89,11 +89,11 @@ public class TestPluginConfigModule { return false; } - @Override - public List> getTransportPreferences() { - return emptyList(); + public Map> getTransportPreferences() { + return emptyMap(); } + }; return pluginConfig; } diff --git a/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java b/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java index 05e28499d..53578ccc4 100644 --- a/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java +++ b/bramble-java/src/main/java/org/briarproject/bramble/plugin/DesktopPluginModule.java @@ -1,6 +1,5 @@ package org.briarproject.bramble.plugin; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.io.TimeoutMonitor; import org.briarproject.bramble.api.lifecycle.IoExecutor; @@ -22,6 +21,7 @@ import org.briarproject.bramble.plugin.tcp.WanTcpPluginFactory; import java.security.SecureRandom; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.concurrent.Executor; import dagger.Module; @@ -30,6 +30,7 @@ import dagger.Provides; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; @Module public class DesktopPluginModule extends PluginModule { @@ -69,10 +70,10 @@ public class DesktopPluginModule extends PluginModule { } @Override - public List> getTransportPreferences() { + public Map> getTransportPreferences() { // Prefer LAN to Bluetooth - return singletonList( - new Pair<>(LanTcpConstants.ID, BluetoothConstants.ID)); + return singletonMap(BluetoothConstants.ID, + singletonList(LanTcpConstants.ID)); } }; return pluginConfig; diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java index d2dace83c..27a720d7f 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java @@ -8,7 +8,6 @@ import android.os.StrictMode; import com.vanniktech.emoji.RecentEmoji; import org.briarproject.bramble.api.FeatureFlags; -import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.battery.BatteryManager; import org.briarproject.bramble.api.crypto.CryptoComponent; import org.briarproject.bramble.api.crypto.KeyStrengthener; @@ -53,6 +52,7 @@ import java.security.GeneralSecurityException; import java.security.SecureRandom; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; @@ -68,6 +68,7 @@ import static android.os.Build.VERSION.SDK_INT; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_ONION_ADDRESS; import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_PUBLIC_KEY_HEX; import static org.briarproject.briar.android.TestingConstants.IS_DEBUG_BUILD; @@ -160,12 +161,11 @@ public class AppModule { return true; } - @Override - public List> getTransportPreferences() { + public Map> getTransportPreferences() { // Prefer LAN to Bluetooth - return singletonList( - new Pair<>(LanTcpConstants.ID, BluetoothConstants.ID)); + return singletonMap(BluetoothConstants.ID, + singletonList(LanTcpConstants.ID)); } }; return pluginConfig; diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt index f1b354192..5cfdac462 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt @@ -4,7 +4,6 @@ import com.fasterxml.jackson.databind.ObjectMapper import dagger.Module import dagger.Provides import org.briarproject.bramble.api.FeatureFlags -import org.briarproject.bramble.api.Pair import org.briarproject.bramble.api.battery.BatteryManager import org.briarproject.bramble.api.db.DatabaseConfig import org.briarproject.bramble.api.event.EventBus @@ -33,7 +32,7 @@ import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File import java.util.Collections.emptyList -import java.util.Collections.singletonList +import java.util.Collections.singletonMap import java.util.concurrent.Executor import javax.inject.Singleton import javax.net.SocketFactory @@ -89,8 +88,9 @@ internal class HeadlessModule(private val appDir: File) { override fun getDuplexFactories(): Collection = duplex override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = true - override fun getTransportPreferences(): List> = - singletonList(Pair(LanTcpConstants.ID, BluetoothConstants.ID)) + // Prefer LAN to Bluetooth + override fun getTransportPreferences(): Map> = + singletonMap(BluetoothConstants.ID, listOf(LanTcpConstants.ID)) } } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt index 4d4bee12c..f02dc5799 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt @@ -4,7 +4,6 @@ import com.fasterxml.jackson.databind.ObjectMapper import dagger.Module import dagger.Provides import org.briarproject.bramble.api.FeatureFlags -import org.briarproject.bramble.api.Pair import org.briarproject.bramble.api.db.DatabaseConfig import org.briarproject.bramble.api.plugin.BluetoothConstants import org.briarproject.bramble.api.plugin.LanTcpConstants @@ -22,8 +21,8 @@ import org.briarproject.briar.headless.event.HeadlessEventModule import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File +import java.util.* import java.util.Collections.emptyList -import java.util.Collections.singletonList import javax.inject.Singleton @Module( @@ -60,8 +59,9 @@ internal class HeadlessTestModule(private val appDir: File) { override fun getDuplexFactories(): Collection = emptyList() override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = false - override fun getTransportPreferences(): List> = - singletonList(Pair(LanTcpConstants.ID, BluetoothConstants.ID)) + // Prefer LAN to Bluetooth + override fun getTransportPreferences(): Map> = + Collections.singletonMap(BluetoothConstants.ID, listOf(LanTcpConstants.ID)) } } From 6eb77465f6ce59c9efcb41f0fbc46278b89b168d Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 25 May 2020 17:15:00 +0100 Subject: [PATCH 16/21] Don't try to reconnect if the connection was closed cleanly. --- .../api/connection/ConnectionRegistry.java | 2 +- .../plugin/event/ConnectionClosedEvent.java | 9 +- .../connection/ConnectionRegistryImpl.java | 8 +- .../IncomingDuplexSyncConnection.java | 5 +- .../OutgoingDuplexSyncConnection.java | 5 +- .../bramble/plugin/PollerImpl.java | 4 +- .../ConnectionRegistryImplTest.java | 11 +- .../bramble/plugin/PollerImplTest.java | 127 +++++++++++++----- 8 files changed, 116 insertions(+), 55 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java index 1b552101e..db5968902 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java @@ -47,7 +47,7 @@ public interface ConnectionRegistry { * the contact. */ void unregisterConnection(ContactId c, TransportId t, - InterruptibleConnection conn, boolean incoming); + InterruptibleConnection conn, boolean incoming, boolean exception); /** * Sets the {@link Priority priority} of a connection that was previously diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/event/ConnectionClosedEvent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/event/ConnectionClosedEvent.java index 05c4cbdd7..6d5e5293b 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/event/ConnectionClosedEvent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/plugin/event/ConnectionClosedEvent.java @@ -13,13 +13,14 @@ public class ConnectionClosedEvent extends Event { private final ContactId contactId; private final TransportId transportId; - private final boolean incoming; + private final boolean incoming, exception; public ConnectionClosedEvent(ContactId contactId, TransportId transportId, - boolean incoming) { + boolean incoming, boolean exception) { this.contactId = contactId; this.transportId = transportId; this.incoming = incoming; + this.exception = exception; } public ContactId getContactId() { @@ -33,4 +34,8 @@ public class ConnectionClosedEvent extends Event { public boolean isIncoming() { return incoming; } + + public boolean isException() { + return exception; + } } 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 d735a7c2f..3b3d11e8b 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 @@ -136,7 +136,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { @Override public void unregisterConnection(ContactId c, TransportId t, - InterruptibleConnection conn, boolean incoming) { + InterruptibleConnection conn, boolean incoming, boolean exception) { if (LOG.isLoggable(INFO)) { if (incoming) LOG.info("Incoming connection unregistered: " + t); else LOG.info("Outgoing connection unregistered: " + t); @@ -148,7 +148,8 @@ class ConnectionRegistryImpl implements ConnectionRegistry { throw new IllegalArgumentException(); lastConnection = recs.isEmpty(); } - eventBus.broadcast(new ConnectionClosedEvent(c, t, incoming)); + eventBus.broadcast( + new ConnectionClosedEvent(c, t, incoming, exception)); if (lastConnection) { LOG.info("Contact disconnected"); eventBus.broadcast(new ContactDisconnectedEvent(c)); @@ -178,8 +179,7 @@ class ConnectionRegistryImpl implements ConnectionRegistry { @Override public Collection getConnectedOrBetterContacts(TransportId t) { synchronized (lock) { - List better = betterTransports.get(t); - if (better == null) better = emptyList(); + List better = getBetterTransports(t); List contactIds = new ArrayList<>(); for (Entry> e : contactConnections.entrySet()) { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java index f1b53e768..f1fa9fe2c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java @@ -74,12 +74,13 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection createIncomingSession(ctx, reader, handler).run(); reader.dispose(false, true); interruptOutgoingSession(); + connectionRegistry.unregisterConnection(contactId, transportId, + this, true, false); } catch (DbException | IOException e) { logException(LOG, WARNING, e); onReadError(true); - } finally { connectionRegistry.unregisterConnection(contactId, transportId, - this, true); + this, true, true); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java index a4d237071..958f651dc 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java @@ -118,12 +118,13 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection createIncomingSession(ctx, reader, handler).run(); reader.dispose(false, true); interruptOutgoingSession(); + connectionRegistry.unregisterConnection(contactId, transportId, + this, false, false); } catch (DbException | IOException e) { logException(LOG, WARNING, e); onReadError(); - } finally { connectionRegistry.unregisterConnection(contactId, transportId, - this, false); + this, false, true); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java index a0b72dc55..1aeb66172 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/plugin/PollerImpl.java @@ -98,8 +98,8 @@ class PollerImpl implements Poller, EventListener { ConnectionClosedEvent c = (ConnectionClosedEvent) e; // Reschedule polling, the polling interval may have decreased reschedule(c.getTransportId()); - if (!c.isIncoming()) { - // Connect to the disconnected contact + // If an outgoing connection failed, try to reconnect + if (!c.isIncoming() && c.isException()) { connectToContact(c.getContactId(), c.getTransportId()); } } else if (e instanceof ConnectionOpenedEvent) { 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 1a1fbc6b1..3341efdb9 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 @@ -108,7 +108,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); }}); - c.unregisterConnection(contactId1, transportId1, conn1, true); + c.unregisterConnection(contactId1, transportId1, conn1, true, false); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -123,7 +123,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(eventBus).broadcast(with(any( ContactDisconnectedEvent.class))); }}); - c.unregisterConnection(contactId1, transportId1, conn2, true); + c.unregisterConnection(contactId1, transportId1, conn2, true, false); context.assertIsSatisfied(); assertEquals(emptyList(), c.getConnectedContacts(transportId1)); @@ -131,7 +131,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { // Try to unregister the connection again - exception should be thrown try { - c.unregisterConnection(contactId1, transportId1, conn2, true); + c.unregisterConnection(contactId1, transportId1, conn2, + true, false); fail(); } catch (IllegalArgumentException expected) { // Expected @@ -332,7 +333,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); }}); - c.unregisterConnection(contactId1, transportId2, conn2, true); + c.unregisterConnection(contactId1, transportId2, conn2, true, false); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -429,7 +430,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionClosedEvent.class))); }}); - c.unregisterConnection(contactId1, transportId1, conn1, true); + c.unregisterConnection(contactId1, transportId1, conn1, true, false); context.assertIsSatisfied(); // The contact is not connected via transport 1 but is connected via a diff --git a/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java index 9d56b2ef4..cf29a7a0e 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/plugin/PollerImplTest.java @@ -157,7 +157,21 @@ public class PollerImplTest extends BrambleMockTestCase { } @Test - public void testRescheduleAndReconnectOnConnectionClosed() + public void testRescheduleOnOutgoingConnectionClosed() { + DuplexPlugin plugin = context.mock(DuplexPlugin.class); + + context.checking(new Expectations() {{ + allowing(plugin).getId(); + will(returnValue(transportId)); + }}); + expectReschedule(plugin); + + poller.eventOccurred(new ConnectionClosedEvent(contactId, transportId, + false, false)); + } + + @Test + public void testRescheduleAndReconnectOnOutgoingConnectionFailed() throws Exception { DuplexPlugin plugin = context.mock(DuplexPlugin.class); DuplexTransportConnection duplexConnection = @@ -166,45 +180,40 @@ public class PollerImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ allowing(plugin).getId(); will(returnValue(transportId)); - // reschedule() - // Get the plugin - oneOf(pluginManager).getPlugin(transportId); - will(returnValue(plugin)); - // The plugin supports polling - oneOf(plugin).shouldPoll(); - will(returnValue(true)); - // Get the plugin - oneOf(pluginManager).getPlugin(transportId); - will(returnValue(plugin)); - // The plugin supports polling - oneOf(plugin).shouldPoll(); - will(returnValue(true)); - // Schedule the next poll - oneOf(plugin).getPollingInterval(); - will(returnValue(pollingInterval)); - oneOf(clock).currentTimeMillis(); - will(returnValue(now)); - oneOf(scheduler).schedule(with(any(Runnable.class)), - with((long) pollingInterval), with(MILLISECONDS)); - will(returnValue(future)); - // connectToContact() - // Check whether the contact is already connected - oneOf(connectionRegistry).isConnected(contactId, transportId); - will(returnValue(false)); - // Get the transport properties - oneOf(transportPropertyManager).getRemoteProperties(contactId, - transportId); - will(returnValue(properties)); - // Connect to the contact - oneOf(plugin).createConnection(properties); - will(returnValue(duplexConnection)); - // Pass the connection to the connection manager - oneOf(connectionManager).manageOutgoingConnection(contactId, - transportId, duplexConnection); }}); + expectReschedule(plugin); + expectReconnect(plugin, duplexConnection); poller.eventOccurred(new ConnectionClosedEvent(contactId, transportId, - false)); + false, true)); + } + + @Test + public void testRescheduleOnIncomingConnectionClosed() { + DuplexPlugin plugin = context.mock(DuplexPlugin.class); + + context.checking(new Expectations() {{ + allowing(plugin).getId(); + will(returnValue(transportId)); + }}); + expectReschedule(plugin); + + poller.eventOccurred(new ConnectionClosedEvent(contactId, transportId, + true, false)); + } + + @Test + public void testRescheduleOnIncomingConnectionFailed() { + DuplexPlugin plugin = context.mock(DuplexPlugin.class); + + context.checking(new Expectations() {{ + allowing(plugin).getId(); + will(returnValue(transportId)); + }}); + expectReschedule(plugin); + + poller.eventOccurred(new ConnectionClosedEvent(contactId, transportId, + true, false)); } @Test @@ -431,4 +440,48 @@ public class PollerImplTest extends BrambleMockTestCase { poller.eventOccurred(new TransportEnabledEvent(transportId)); poller.eventOccurred(new TransportDisabledEvent(transportId)); } + + private void expectReschedule(Plugin plugin) { + context.checking(new Expectations() {{ + // Get the plugin + oneOf(pluginManager).getPlugin(transportId); + will(returnValue(plugin)); + // The plugin supports polling + oneOf(plugin).shouldPoll(); + will(returnValue(true)); + // Schedule the next poll + oneOf(plugin).getPollingInterval(); + will(returnValue(pollingInterval)); + oneOf(clock).currentTimeMillis(); + will(returnValue(now)); + oneOf(scheduler).schedule(with(any(Runnable.class)), + with((long) pollingInterval), with(MILLISECONDS)); + will(returnValue(future)); + }}); + } + + private void expectReconnect(DuplexPlugin plugin, + DuplexTransportConnection duplexConnection) throws Exception { + context.checking(new Expectations() {{ + // Get the plugin + oneOf(pluginManager).getPlugin(transportId); + will(returnValue(plugin)); + // The plugin supports polling + oneOf(plugin).shouldPoll(); + will(returnValue(true)); + // Check whether the contact is already connected + oneOf(connectionRegistry).isConnected(contactId, transportId); + will(returnValue(false)); + // Get the transport properties + oneOf(transportPropertyManager).getRemoteProperties(contactId, + transportId); + will(returnValue(properties)); + // Connect to the contact + oneOf(plugin).createConnection(properties); + will(returnValue(duplexConnection)); + // Pass the connection to the connection manager + oneOf(connectionManager).manageOutgoingConnection(contactId, + transportId, duplexConnection); + }}); + } } From cc943be5403c01469ca731e2ddd476f5ab83a914 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 1 Jun 2020 15:30:30 +0100 Subject: [PATCH 17/21] Update javadoc. --- .../api/connection/ConnectionRegistry.java | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java index db5968902..4ff3eb5ef 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java @@ -24,14 +24,6 @@ public interface ConnectionRegistry { /** * Registers a connection with the given contact over the given transport. *

- * If the registry has any connections with the same contact and a - * {@link PluginConfig#getTransportPreferences() worse} transport, those - * connections will be - * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. - *

- * If the registry has any connections with the same contact and a better - * transport, the given connection will be interrupted. - *

* Broadcasts {@link ConnectionOpenedEvent}. Also broadcasts * {@link ContactConnectedEvent} if this is the only connection with the * contact. @@ -54,12 +46,19 @@ public interface ConnectionRegistry { * registered via {@link #registerConnection(ContactId, TransportId, * InterruptibleConnection, boolean)}. *

- * If the registry has any connections with the same contact and transport - * and a lower {@link Priority priority}, those connections will be - * {@link InterruptibleConnection#interruptOutgoingSession() interrupted}. + * If the registry has any "better" connections with the given contact, the + * given connection will be interrupted. If the registry has any "worse" + * connections with the given contact, those connections will be + * interrupted. *

- * If the registry has any connections with the same contact and transport - * and a higher priority, the given connection will be interrupted. + * Connection A is considered "better" than connection B if both + * connections have had their priorities set, and either A's transport is + * {@link PluginConfig#getTransportPreferences() preferred} to B's, or + * they use the same transport and A has higher {@link Priority priority} + * than B. + *

+ * For backward compatibility, connections without priorities are not + * considered better or worse than other connections. */ void setPriority(ContactId c, TransportId t, InterruptibleConnection conn, Priority priority); From 78d7fc21060daec447573845199300807a14bb49 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Tue, 2 Jun 2020 12:00:06 +0100 Subject: [PATCH 18/21] Fix bug in reporting of connection state, add regression tests. --- .../connection/ConnectionRegistryImpl.java | 3 ++- .../ConnectionRegistryImplTest.java | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) 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 3b3d11e8b..897ef0880 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 @@ -214,7 +214,8 @@ class ConnectionRegistryImpl implements ConnectionRegistry { @Override public boolean isConnected(ContactId c) { synchronized (lock) { - return contactConnections.containsKey(c); + List recs = contactConnections.get(c); + return recs != null && !recs.isEmpty(); } } 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 3341efdb9..6a2821219 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 @@ -74,6 +74,10 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId2)); assertEquals(emptyList(), c.getConnectedContacts(transportId3)); assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId3)); + assertFalse(c.isConnected(contactId1)); + assertFalse(c.isConnected(contactId1, transportId1)); + assertFalse(c.isConnected(contactId1, transportId2)); + assertFalse(c.isConnected(contactId1, transportId3)); // Check that a registered connection shows up - this should // broadcast a ConnectionOpenedEvent and a ContactConnectedEvent @@ -88,6 +92,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { c.getConnectedContacts(transportId1)); assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId1)); + assertTrue(c.isConnected(contactId1)); + assertTrue(c.isConnected(contactId1, transportId1)); // Register another connection with the same contact and transport - // this should broadcast a ConnectionOpenedEvent and lookup should be @@ -102,6 +108,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { c.getConnectedContacts(transportId1)); assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId1)); + assertTrue(c.isConnected(contactId1)); + assertTrue(c.isConnected(contactId1, transportId1)); // Unregister one of the connections - this should broadcast a // ConnectionClosedEvent and lookup should be unaffected @@ -115,6 +123,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { c.getConnectedContacts(transportId1)); assertEquals(singletonList(contactId1), c.getConnectedOrBetterContacts(transportId1)); + assertTrue(c.isConnected(contactId1)); + assertTrue(c.isConnected(contactId1, transportId1)); // Unregister the other connection - this should broadcast a // ConnectionClosedEvent and a ContactDisconnectedEvent @@ -128,6 +138,8 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { assertEquals(emptyList(), c.getConnectedContacts(transportId1)); assertEquals(emptyList(), c.getConnectedOrBetterContacts(transportId1)); + assertFalse(c.isConnected(contactId1)); + assertFalse(c.isConnected(contactId1, transportId1)); // Try to unregister the connection again - exception should be thrown try { @@ -163,6 +175,15 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { c.registerConnection(contactId2, transportId2, conn3, true); context.assertIsSatisfied(); + assertTrue(c.isConnected(contactId1)); + assertTrue(c.isConnected(contactId2)); + + assertTrue(c.isConnected(contactId1, transportId1)); + assertFalse(c.isConnected(contactId1, transportId2)); + + assertTrue(c.isConnected(contactId2, transportId1)); + assertTrue(c.isConnected(contactId2, transportId2)); + Collection connected = c.getConnectedContacts(transportId1); assertEquals(2, connected.size()); assertTrue(connected.contains(contactId1)); From 95f427863db6500feda22f0a9101161e4aba2fcf Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 25 Jun 2020 17:46:22 +0100 Subject: [PATCH 19/21] Remove transport preferences for briar-headless. --- .../org/briarproject/briar/headless/HeadlessModule.kt | 9 ++++----- .../briarproject/briar/headless/HeadlessTestModule.kt | 7 +------ 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt index 5cfdac462..c74b36225 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/HeadlessModule.kt @@ -9,7 +9,9 @@ import org.briarproject.bramble.api.db.DatabaseConfig import org.briarproject.bramble.api.event.EventBus import org.briarproject.bramble.api.lifecycle.IoExecutor import org.briarproject.bramble.api.network.NetworkManager -import org.briarproject.bramble.api.plugin.* +import org.briarproject.bramble.api.plugin.BackoffFactory +import org.briarproject.bramble.api.plugin.PluginConfig +import org.briarproject.bramble.api.plugin.TransportId import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory import org.briarproject.bramble.api.plugin.simplex.SimplexPluginFactory import org.briarproject.bramble.api.system.Clock @@ -32,7 +34,6 @@ import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File import java.util.Collections.emptyList -import java.util.Collections.singletonMap import java.util.concurrent.Executor import javax.inject.Singleton import javax.net.SocketFactory @@ -88,9 +89,7 @@ internal class HeadlessModule(private val appDir: File) { override fun getDuplexFactories(): Collection = duplex override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = true - // Prefer LAN to Bluetooth - override fun getTransportPreferences(): Map> = - singletonMap(BluetoothConstants.ID, listOf(LanTcpConstants.ID)) + override fun getTransportPreferences(): Map> = emptyMap() } } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt index f02dc5799..8482441e5 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/HeadlessTestModule.kt @@ -5,8 +5,6 @@ import dagger.Module import dagger.Provides import org.briarproject.bramble.api.FeatureFlags import org.briarproject.bramble.api.db.DatabaseConfig -import org.briarproject.bramble.api.plugin.BluetoothConstants -import org.briarproject.bramble.api.plugin.LanTcpConstants import org.briarproject.bramble.api.plugin.PluginConfig import org.briarproject.bramble.api.plugin.TransportId import org.briarproject.bramble.api.plugin.duplex.DuplexPluginFactory @@ -21,7 +19,6 @@ import org.briarproject.briar.headless.event.HeadlessEventModule import org.briarproject.briar.headless.forums.HeadlessForumModule import org.briarproject.briar.headless.messaging.HeadlessMessagingModule import java.io.File -import java.util.* import java.util.Collections.emptyList import javax.inject.Singleton @@ -59,9 +56,7 @@ internal class HeadlessTestModule(private val appDir: File) { override fun getDuplexFactories(): Collection = emptyList() override fun getSimplexFactories(): Collection = emptyList() override fun shouldPoll(): Boolean = false - // Prefer LAN to Bluetooth - override fun getTransportPreferences(): Map> = - Collections.singletonMap(BluetoothConstants.ID, listOf(LanTcpConstants.ID)) + override fun getTransportPreferences(): Map> = emptyMap() } } From 7736a3b6fcb34586e1a47644a62ffb68a57fbfe9 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 26 Jun 2020 09:59:03 +0100 Subject: [PATCH 20/21] Use separate methods for registering incoming and outgoing connections. --- .../api/connection/ConnectionRegistry.java | 38 ++++++++++++-- .../connection/ConnectionRegistryImpl.java | 20 +++++++- .../IncomingDuplexSyncConnection.java | 4 +- .../OutgoingDuplexSyncConnection.java | 5 +- .../ConnectionRegistryImplTest.java | 50 ++++++++----------- 5 files changed, 76 insertions(+), 41 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java index 4ff3eb5ef..d23b0144a 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/connection/ConnectionRegistry.java @@ -22,14 +22,42 @@ import java.util.Collection; public interface ConnectionRegistry { /** - * Registers a connection with the given contact over the given transport. + * Registers an incoming connection from the given contact over the given + * transport. The connection's {@link Priority priority} can be set later + * via {@link #setPriority(ContactId, TransportId, InterruptibleConnection, + * Priority)} if a priority record is received from the contact. *

* Broadcasts {@link ConnectionOpenedEvent}. Also broadcasts * {@link ContactConnectedEvent} if this is the only connection with the * contact. */ - void registerConnection(ContactId c, TransportId t, - InterruptibleConnection conn, boolean incoming); + void registerIncomingConnection(ContactId c, TransportId t, + InterruptibleConnection conn); + + /** + * Registers an outgoing connection to the given contact over the given + * transport. + *

+ * Broadcasts {@link ConnectionOpenedEvent}. Also broadcasts + * {@link ContactConnectedEvent} if this is the only connection with the + * contact. + *

+ * If the registry has any "better" connections with the given contact, the + * given connection will be interrupted. If the registry has any "worse" + * connections with the given contact, those connections will be + * interrupted. + *

+ * Connection A is considered "better" than connection B if both + * connections have had their priorities set, and either A's transport is + * {@link PluginConfig#getTransportPreferences() preferred} to B's, or + * they use the same transport and A has higher {@link Priority priority} + * than B. + *

+ * For backward compatibility, connections without priorities are not + * considered better or worse than other connections. + */ + void registerOutgoingConnection(ContactId c, TransportId t, + InterruptibleConnection conn, Priority priority); /** * Unregisters a connection with the given contact over the given transport. @@ -43,8 +71,8 @@ public interface ConnectionRegistry { /** * Sets the {@link Priority priority} of a connection that was previously - * registered via {@link #registerConnection(ContactId, TransportId, - * InterruptibleConnection, boolean)}. + * registered via {@link #registerIncomingConnection(ContactId, TransportId, + * InterruptibleConnection)}. *

* If the registry has any "better" connections with the given contact, the * given connection will be interrupted. If the registry has any "worse" 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 897ef0880..bf33c7387 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 @@ -61,7 +61,25 @@ class ConnectionRegistryImpl implements ConnectionRegistry { } @Override - public void registerConnection(ContactId c, TransportId t, + public void registerIncomingConnection(ContactId c, TransportId t, + InterruptibleConnection conn) { + if (LOG.isLoggable(INFO)) { + LOG.info("Incoming connection registered: " + t); + } + registerConnection(c, t, conn, true); + } + + @Override + public void registerOutgoingConnection(ContactId c, TransportId t, + InterruptibleConnection conn, Priority priority) { + if (LOG.isLoggable(INFO)) { + LOG.info("Outgoing connection registered: " + t); + } + registerConnection(c, t, conn, false); + setPriority(c, t, conn, priority); + } + + private void registerConnection(ContactId c, TransportId t, InterruptibleConnection conn, boolean incoming) { if (LOG.isLoggable(INFO)) { if (incoming) LOG.info("Incoming connection registered: " + t); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java index f1fa9fe2c..abf34ca0f 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/IncomingDuplexSyncConnection.java @@ -59,8 +59,8 @@ class IncomingDuplexSyncConnection extends DuplexSyncConnection onReadError(true); return; } - connectionRegistry.registerConnection(contactId, transportId, - this, true); + connectionRegistry.registerIncomingConnection(contactId, transportId, + this); // Start the outgoing session on another thread ioExecutor.execute(() -> runOutgoingSession(contactId)); try { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java index 958f651dc..d7f777b7b 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/connection/OutgoingDuplexSyncConnection.java @@ -104,9 +104,8 @@ class OutgoingDuplexSyncConnection extends DuplexSyncConnection onReadError(); return; } - connectionRegistry.registerConnection(contactId, transportId, - this, false); - connectionRegistry.setPriority(contactId, transportId, this, priority); + connectionRegistry.registerOutgoingConnection(contactId, transportId, + this, priority); try { // Store any transport properties discovered from the connection transportPropertyManager.addRemotePropertiesFromConnection( 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 6a2821219..c8b6de86d 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 @@ -85,7 +85,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); oneOf(eventBus).broadcast(with(any(ContactConnectedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn1, true); + c.registerIncomingConnection(contactId1, transportId1, conn1); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -101,7 +101,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn2, true); + c.registerIncomingConnection(contactId1, transportId1, conn2); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -170,9 +170,9 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { exactly(2).of(eventBus).broadcast(with(any( ContactConnectedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn1, true); - c.registerConnection(contactId2, transportId1, conn2, true); - c.registerConnection(contactId2, transportId2, conn3, true); + c.registerIncomingConnection(contactId1, transportId1, conn1); + c.registerIncomingConnection(contactId2, transportId1, conn2); + c.registerIncomingConnection(contactId2, transportId2, conn3); context.assertIsSatisfied(); assertTrue(c.isConnected(contactId1)); @@ -212,12 +212,12 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { ConnectionRegistry c = new ConnectionRegistryImpl(eventBus, pluginConfig); - // Connect via transport 1 (worse than 2) and set priority to low + // Connect via transport 1 (worse than 2) with no priority set 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.registerIncomingConnection(contactId1, transportId1, conn1); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -234,8 +234,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId2, conn2, true); - c.setPriority(contactId1, transportId2, conn2, high); + c.registerOutgoingConnection(contactId1, transportId2, conn2, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -253,8 +252,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId3, conn3, true); - c.setPriority(contactId1, transportId3, conn3, high); + c.registerOutgoingConnection(contactId1, transportId3, conn3, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -290,8 +288,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { 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); + c.registerOutgoingConnection(contactId1, transportId1, conn1, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -312,8 +309,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(conn2).interruptOutgoingSession(); oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId2, conn2, true); - c.setPriority(contactId1, transportId2, conn2, high); + c.registerOutgoingConnection(contactId1, transportId2, conn2, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -331,8 +327,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId3, conn3, true); - c.setPriority(contactId1, transportId3, conn3, low); + c.registerOutgoingConnection(contactId1, transportId3, conn3, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -391,8 +386,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { 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); + c.registerOutgoingConnection(contactId1, transportId1, conn1, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -410,8 +404,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(conn1).interruptOutgoingSession(); oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId2, conn2, true); - c.setPriority(contactId1, transportId2, conn2, low); + c.registerOutgoingConnection(contactId1, transportId2, conn2, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -429,7 +422,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId3, conn3, true); + c.registerOutgoingConnection(contactId1, transportId3, conn3, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -486,8 +479,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { 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); + c.registerOutgoingConnection(contactId1, transportId1, conn1, high); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -499,7 +491,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn2, true); + c.registerIncomingConnection(contactId1, transportId1, conn2); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -526,8 +518,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { oneOf(conn3).interruptOutgoingSession(); oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn3, true); - c.setPriority(contactId1, transportId1, conn3, low); + c.registerOutgoingConnection(contactId1, transportId1, conn3, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -551,8 +542,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { 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); + c.registerOutgoingConnection(contactId1, transportId1, conn1, low); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), @@ -564,7 +554,7 @@ public class ConnectionRegistryImplTest extends BrambleMockTestCase { context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(any(ConnectionOpenedEvent.class))); }}); - c.registerConnection(contactId1, transportId1, conn2, true); + c.registerIncomingConnection(contactId1, transportId1, conn2); context.assertIsSatisfied(); assertEquals(singletonList(contactId1), From 730d553b0a3e0f385c671c5db15467ad44c1fa8d Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 26 Jun 2020 10:38:04 +0100 Subject: [PATCH 21/21] Fix screenshot test (again). --- .../java/org/briarproject/briar/android/SetupDataTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java b/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java index a5771f379..140e54231 100644 --- a/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java +++ b/briar-android/src/androidTestScreenshot/java/org/briarproject/briar/android/SetupDataTest.java @@ -120,8 +120,8 @@ public class SetupDataTest extends ScreenshotTest { // TODO add messages - connectionRegistry.registerConnection(bob.getId(), ID, () -> { - }, true); + connectionRegistry.registerIncomingConnection(bob.getId(), ID, () -> { + }); } }