From 98191fb0594984f5900c99fc66ce7e7178bff823 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 5 Jun 2019 11:57:46 +0100 Subject: [PATCH 1/5] Add ContactManager support for pending contact states. --- .../bramble/contact/ContactManagerImpl.java | 70 +++++++++++++++++-- .../bramble/contact/ContactModule.java | 5 +- .../contact/ContactManagerImplTest.java | 12 +++- 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java index 7c6e77d0a..17f8b49db 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java @@ -8,6 +8,7 @@ import org.briarproject.bramble.api.contact.ContactManager; import org.briarproject.bramble.api.contact.PendingContact; import org.briarproject.bramble.api.contact.PendingContactId; import org.briarproject.bramble.api.contact.PendingContactState; +import org.briarproject.bramble.api.contact.event.PendingContactStateChangedEvent; import org.briarproject.bramble.api.crypto.KeyPair; import org.briarproject.bramble.api.crypto.PublicKey; import org.briarproject.bramble.api.crypto.SecretKey; @@ -15,24 +16,35 @@ import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.NoSuchContactException; import org.briarproject.bramble.api.db.Transaction; +import org.briarproject.bramble.api.event.Event; +import org.briarproject.bramble.api.event.EventBus; +import org.briarproject.bramble.api.event.EventListener; import org.briarproject.bramble.api.identity.Author; import org.briarproject.bramble.api.identity.AuthorId; import org.briarproject.bramble.api.identity.AuthorInfo; import org.briarproject.bramble.api.identity.IdentityManager; import org.briarproject.bramble.api.identity.LocalAuthor; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionClosedEvent; +import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedEvent; +import org.briarproject.bramble.api.rendezvous.event.RendezvousFailedEvent; import org.briarproject.bramble.api.transport.KeyManager; import java.security.GeneralSecurityException; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.CopyOnWriteArrayList; import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; +import static org.briarproject.bramble.api.contact.PendingContactState.ADDING_CONTACT; +import static org.briarproject.bramble.api.contact.PendingContactState.FAILED; import static org.briarproject.bramble.api.contact.PendingContactState.WAITING_FOR_CONNECTION; import static org.briarproject.bramble.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENGTH; import static org.briarproject.bramble.api.identity.AuthorInfo.Status.OURSELVES; @@ -43,24 +55,31 @@ import static org.briarproject.bramble.util.StringUtils.toUtf8; @ThreadSafe @NotNullByDefault -class ContactManagerImpl implements ContactManager { +class ContactManagerImpl implements ContactManager, EventListener { private final DatabaseComponent db; private final KeyManager keyManager; private final IdentityManager identityManager; private final PendingContactFactory pendingContactFactory; + private final EventBus eventBus; - private final List hooks; + private final List hooks = new CopyOnWriteArrayList<>(); + private final Object statesLock = new Object(); + @GuardedBy("statesLock") + private final Map states = + new HashMap<>(); @Inject - ContactManagerImpl(DatabaseComponent db, KeyManager keyManager, + ContactManagerImpl(DatabaseComponent db, + KeyManager keyManager, IdentityManager identityManager, - PendingContactFactory pendingContactFactory) { + PendingContactFactory pendingContactFactory, + EventBus eventBus) { this.db = db; this.keyManager = keyManager; this.identityManager = identityManager; this.pendingContactFactory = pendingContactFactory; - hooks = new CopyOnWriteArrayList<>(); + this.eventBus = eventBus; } @Override @@ -139,6 +158,7 @@ class ContactManagerImpl implements ContactManager { } finally { db.endTransaction(txn); } + setState(p.getId(), WAITING_FOR_CONNECTION); return p; } @@ -156,7 +176,12 @@ class ContactManagerImpl implements ContactManager { List> pairs = new ArrayList<>(pendingContacts.size()); for (PendingContact p : pendingContacts) { - pairs.add(new Pair<>(p, WAITING_FOR_CONNECTION)); // TODO + PendingContactState state; + synchronized (statesLock) { + state = states.get(p.getId()); + } + if (state == null) state = WAITING_FOR_CONNECTION; + pairs.add(new Pair<>(p, state)); } return pairs; } @@ -164,6 +189,9 @@ class ContactManagerImpl implements ContactManager { @Override public void removePendingContact(PendingContactId p) throws DbException { db.transaction(false, txn -> db.removePendingContact(txn, p)); + synchronized (statesLock) { + states.remove(p); + } } @Override @@ -263,4 +291,34 @@ class ContactManagerImpl implements ContactManager { else return new AuthorInfo(UNVERIFIED, c.getAlias()); } + @Override + public void eventOccurred(Event e) { + if (e instanceof RendezvousConnectionOpenedEvent) { + RendezvousConnectionOpenedEvent r = + (RendezvousConnectionOpenedEvent) e; + setState(r.getPendingContactId(), ADDING_CONTACT); + } else if (e instanceof RendezvousConnectionClosedEvent) { + RendezvousConnectionClosedEvent r = + (RendezvousConnectionClosedEvent) e; + // We're only interested in failures - if the rendezvous succeeds + // the pending contact will be removed + if (!r.isSuccess()) + setState(r.getPendingContactId(), WAITING_FOR_CONNECTION); + } else if (e instanceof RendezvousFailedEvent) { + RendezvousFailedEvent r = (RendezvousFailedEvent) e; + setState(r.getPendingContactId(), FAILED); + } + } + + /* + * Sets the state of the given pending contact and broadcasts an event, + * unless the current state is FAILED. + */ + private void setState(PendingContactId p, PendingContactState state) { + synchronized (statesLock) { + if (states.get(p) == FAILED) return; + states.put(p, state); + eventBus.broadcast(new PendingContactStateChangedEvent(p, state)); + } + } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactModule.java b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactModule.java index f4d6df9f3..f0b5b6259 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactModule.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactModule.java @@ -3,6 +3,7 @@ package org.briarproject.bramble.contact; import org.briarproject.bramble.api.contact.ContactExchangeManager; import org.briarproject.bramble.api.contact.ContactManager; import org.briarproject.bramble.api.contact.HandshakeManager; +import org.briarproject.bramble.api.event.EventBus; import javax.inject.Inject; import javax.inject.Singleton; @@ -20,7 +21,9 @@ public class ContactModule { @Provides @Singleton - ContactManager provideContactManager(ContactManagerImpl contactManager) { + ContactManager provideContactManager(EventBus eventBus, + ContactManagerImpl contactManager) { + eventBus.addListener(contactManager); return contactManager; } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java index 1eab88e88..a96f730fc 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java @@ -11,6 +11,7 @@ import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.NoSuchContactException; import org.briarproject.bramble.api.db.Transaction; +import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.identity.Author; import org.briarproject.bramble.api.identity.AuthorId; import org.briarproject.bramble.api.identity.AuthorInfo; @@ -20,6 +21,7 @@ import org.briarproject.bramble.api.transport.KeyManager; import org.briarproject.bramble.test.BrambleMockTestCase; import org.briarproject.bramble.test.DbExpectations; import org.jmock.Expectations; +import org.junit.Before; import org.junit.Test; import java.util.Collection; @@ -54,7 +56,8 @@ public class ContactManagerImplTest extends BrambleMockTestCase { context.mock(IdentityManager.class); private final PendingContactFactory pendingContactFactory = context.mock(PendingContactFactory.class); - private final ContactManager contactManager; + private final EventBus eventBus = context.mock(EventBus.class); + private final Author remote = getAuthor(); private final LocalAuthor localAuthor = getLocalAuthor(); private final AuthorId local = localAuthor.getId(); @@ -62,9 +65,12 @@ public class ContactManagerImplTest extends BrambleMockTestCase { private final Contact contact = getContact(remote, local, verified); private final ContactId contactId = contact.getId(); - public ContactManagerImplTest() { + private ContactManager contactManager; + + @Before + public void setUp() { contactManager = new ContactManagerImpl(db, keyManager, - identityManager, pendingContactFactory); + identityManager, pendingContactFactory, eventBus); } @Test From 1aa579a44f7962874906d1c835e59618a4c51b33 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 5 Jun 2019 12:23:15 +0100 Subject: [PATCH 2/5] Add unit tests for pending contact state. --- .../contact/ContactManagerImplTest.java | 108 ++++++++++++++++-- 1 file changed, 99 insertions(+), 9 deletions(-) diff --git a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java index a96f730fc..543822a3e 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java @@ -1,11 +1,12 @@ package org.briarproject.bramble.contact; +import org.briarproject.bramble.api.Pair; import org.briarproject.bramble.api.contact.Contact; import org.briarproject.bramble.api.contact.ContactId; -import org.briarproject.bramble.api.contact.ContactManager; +import org.briarproject.bramble.api.contact.PendingContact; +import org.briarproject.bramble.api.contact.PendingContactState; +import org.briarproject.bramble.api.contact.event.PendingContactStateChangedEvent; import org.briarproject.bramble.api.crypto.KeyPair; -import org.briarproject.bramble.api.crypto.PrivateKey; -import org.briarproject.bramble.api.crypto.PublicKey; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DbException; @@ -17,8 +18,12 @@ import org.briarproject.bramble.api.identity.AuthorId; import org.briarproject.bramble.api.identity.AuthorInfo; import org.briarproject.bramble.api.identity.IdentityManager; import org.briarproject.bramble.api.identity.LocalAuthor; +import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionClosedEvent; +import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedEvent; +import org.briarproject.bramble.api.rendezvous.event.RendezvousFailedEvent; import org.briarproject.bramble.api.transport.KeyManager; import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.CaptureArgumentAction; import org.briarproject.bramble.test.DbExpectations; import org.jmock.Expectations; import org.junit.Before; @@ -26,10 +31,14 @@ import org.junit.Test; import java.util.Collection; import java.util.Random; +import java.util.concurrent.atomic.AtomicReference; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static org.briarproject.bramble.api.contact.HandshakeLinkConstants.BASE32_LINK_BYTES; +import static org.briarproject.bramble.api.contact.PendingContactState.ADDING_CONTACT; +import static org.briarproject.bramble.api.contact.PendingContactState.FAILED; +import static org.briarproject.bramble.api.contact.PendingContactState.WAITING_FOR_CONNECTION; import static org.briarproject.bramble.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENGTH; import static org.briarproject.bramble.api.identity.AuthorInfo.Status.OURSELVES; import static org.briarproject.bramble.api.identity.AuthorInfo.Status.UNKNOWN; @@ -40,6 +49,7 @@ import static org.briarproject.bramble.test.TestUtils.getAgreementPublicKey; import static org.briarproject.bramble.test.TestUtils.getAuthor; import static org.briarproject.bramble.test.TestUtils.getContact; import static org.briarproject.bramble.test.TestUtils.getLocalAuthor; +import static org.briarproject.bramble.test.TestUtils.getPendingContact; import static org.briarproject.bramble.test.TestUtils.getRandomId; import static org.briarproject.bramble.test.TestUtils.getSecretKey; import static org.briarproject.bramble.util.StringUtils.getRandomBase32String; @@ -64,8 +74,11 @@ public class ContactManagerImplTest extends BrambleMockTestCase { private final boolean verified = false, active = true; private final Contact contact = getContact(remote, local, verified); private final ContactId contactId = contact.getId(); + private final KeyPair handshakeKeyPair = + new KeyPair(getAgreementPublicKey(), getAgreementPrivateKey()); + private final PendingContact pendingContact = getPendingContact(); - private ContactManager contactManager; + private ContactManagerImpl contactManager; @Before public void setUp() { @@ -97,6 +110,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testGetContact() throws Exception { Transaction txn = new Transaction(null, true); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).getContact(txn, contactId); @@ -110,6 +124,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { public void testGetContactByAuthor() throws Exception { Transaction txn = new Transaction(null, true); Collection contacts = singletonList(contact); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).getContactsByAuthorId(txn, remote.getId()); @@ -122,6 +137,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test(expected = NoSuchContactException.class) public void testGetContactByUnknownAuthor() throws Exception { Transaction txn = new Transaction(null, true); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).getContactsByAuthorId(txn, remote.getId()); @@ -135,6 +151,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { public void testGetContactByUnknownLocalAuthor() throws Exception { Transaction txn = new Transaction(null, true); Collection contacts = singletonList(contact); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).getContactsByAuthorId(txn, remote.getId()); @@ -148,6 +165,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { public void testGetContacts() throws Exception { Collection contacts = singletonList(contact); Transaction txn = new Transaction(null, true); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).getContacts(txn); @@ -160,6 +178,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testRemoveContact() throws Exception { Transaction txn = new Transaction(null, false); + context.checking(new DbExpectations() {{ oneOf(db).transaction(with(false), withDbRunnable(txn)); oneOf(db).getContact(txn, contactId); @@ -193,6 +212,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testContactExists() throws Exception { Transaction txn = new Transaction(null, true); + context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(db).containsContact(txn, remote.getId(), local); @@ -212,6 +232,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { oneOf(db).getContactsByAuthorId(txn, remote.getId()); will(returnValue(singletonList(contact))); }}); + AuthorInfo authorInfo = contactManager.getAuthorInfo(txn, remote.getId()); assertEquals(UNVERIFIED, authorInfo.getStatus()); @@ -229,6 +250,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { oneOf(db).getContactsByAuthorId(txn, remote.getId()); will(returnValue(emptyList())); }}); + AuthorInfo authorInfo = contactManager.getAuthorInfo(txn, remote.getId()); assertEquals(UNKNOWN, authorInfo.getStatus()); @@ -253,6 +275,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { will(returnValue(localAuthor)); never(db).getContactsByAuthorId(txn, remote.getId()); }}); + authorInfo = contactManager.getAuthorInfo(txn, localAuthor.getId()); assertEquals(OURSELVES, authorInfo.getStatus()); assertNull(authorInfo.getAlias()); @@ -271,19 +294,86 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testGetHandshakeLink() throws Exception { Transaction txn = new Transaction(null, true); - PublicKey publicKey = getAgreementPublicKey(); - PrivateKey privateKey = getAgreementPrivateKey(); - KeyPair keyPair = new KeyPair(publicKey, privateKey); String link = "briar://" + getRandomBase32String(BASE32_LINK_BYTES); context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); oneOf(identityManager).getHandshakeKeys(txn); - will(returnValue(keyPair)); - oneOf(pendingContactFactory).createHandshakeLink(publicKey); + will(returnValue(handshakeKeyPair)); + oneOf(pendingContactFactory).createHandshakeLink( + handshakeKeyPair.getPublic()); will(returnValue(link)); }}); assertEquals(link, contactManager.getHandshakeLink()); } + + @Test + public void testDefaultPendingContactState() throws Exception { + Transaction txn = new Transaction(null, true); + + context.checking(new DbExpectations() {{ + oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); + oneOf(db).getPendingContacts(txn); + will(returnValue(singletonList(pendingContact))); + }}); + + // No events have happened for this pending contact, so the state + // should be WAITING_FOR_CONNECTION + Collection> pairs = + contactManager.getPendingContacts(); + assertEquals(1, pairs.size()); + Pair pair = + pairs.iterator().next(); + assertEquals(pendingContact, pair.getFirst()); + assertEquals(WAITING_FOR_CONNECTION, pair.getSecond()); + } + + @Test + public void testFailedStateIsNotReplaced() throws Exception { + Transaction txn = new Transaction(null, true); + AtomicReference captureFirstEvent = + new AtomicReference<>(); + AtomicReference captureSecondEvent = + new AtomicReference<>(); + + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(any( + PendingContactStateChangedEvent.class))); + will(new CaptureArgumentAction<>(captureFirstEvent, + PendingContactStateChangedEvent.class, 0)); + oneOf(eventBus).broadcast(with(any( + PendingContactStateChangedEvent.class))); + will(new CaptureArgumentAction<>(captureSecondEvent, + PendingContactStateChangedEvent.class, 0)); + }}); + + // A rendezvous connection is opened, then the pending contact expires, + // then the rendezvous connection is closed + contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( + pendingContact.getId())); + contactManager.eventOccurred(new RendezvousFailedEvent( + pendingContact.getId())); + contactManager.eventOccurred(new RendezvousConnectionClosedEvent( + pendingContact.getId(), false)); + + assertEquals(ADDING_CONTACT, + captureFirstEvent.get().getPendingContactState()); + assertEquals(FAILED, captureSecondEvent.get().getPendingContactState()); + + context.checking(new DbExpectations() {{ + oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); + oneOf(db).getPendingContacts(txn); + will(returnValue(singletonList(pendingContact))); + }}); + + // The FAILED state should not have been overwritten + Collection> pairs = + contactManager.getPendingContacts(); + assertEquals(1, pairs.size()); + Pair pair = + pairs.iterator().next(); + assertEquals(pendingContact, pair.getFirst()); + assertEquals(FAILED, pair.getSecond()); + } } From 23354d6568f250e58283d7698519a4f99f7b9534 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 5 Jun 2019 12:56:39 +0100 Subject: [PATCH 3/5] Use predicates to match events. --- .../contact/ContactManagerImplTest.java | 26 ++++++----------- .../bramble/test/PredicateMatcher.java | 28 +++++++++++++++++++ 2 files changed, 36 insertions(+), 18 deletions(-) create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/test/PredicateMatcher.java diff --git a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java index 543822a3e..48928cefb 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java @@ -23,15 +23,14 @@ import org.briarproject.bramble.api.rendezvous.event.RendezvousConnectionOpenedE import org.briarproject.bramble.api.rendezvous.event.RendezvousFailedEvent; import org.briarproject.bramble.api.transport.KeyManager; import org.briarproject.bramble.test.BrambleMockTestCase; -import org.briarproject.bramble.test.CaptureArgumentAction; import org.briarproject.bramble.test.DbExpectations; +import org.briarproject.bramble.test.PredicateMatcher; import org.jmock.Expectations; import org.junit.Before; import org.junit.Test; import java.util.Collection; import java.util.Random; -import java.util.concurrent.atomic.AtomicReference; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -332,20 +331,14 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testFailedStateIsNotReplaced() throws Exception { Transaction txn = new Transaction(null, true); - AtomicReference captureFirstEvent = - new AtomicReference<>(); - AtomicReference captureSecondEvent = - new AtomicReference<>(); context.checking(new Expectations() {{ - oneOf(eventBus).broadcast(with(any( - PendingContactStateChangedEvent.class))); - will(new CaptureArgumentAction<>(captureFirstEvent, - PendingContactStateChangedEvent.class, 0)); - oneOf(eventBus).broadcast(with(any( - PendingContactStateChangedEvent.class))); - will(new CaptureArgumentAction<>(captureSecondEvent, - PendingContactStateChangedEvent.class, 0)); + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == ADDING_CONTACT))); + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == FAILED))); }}); // A rendezvous connection is opened, then the pending contact expires, @@ -356,10 +349,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { pendingContact.getId())); contactManager.eventOccurred(new RendezvousConnectionClosedEvent( pendingContact.getId(), false)); - - assertEquals(ADDING_CONTACT, - captureFirstEvent.get().getPendingContactState()); - assertEquals(FAILED, captureSecondEvent.get().getPendingContactState()); + context.assertIsSatisfied(); context.checking(new DbExpectations() {{ oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/test/PredicateMatcher.java b/bramble-core/src/test/java/org/briarproject/bramble/test/PredicateMatcher.java new file mode 100644 index 000000000..b08f15b2a --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/test/PredicateMatcher.java @@ -0,0 +1,28 @@ +package org.briarproject.bramble.test; + +import org.briarproject.bramble.api.Predicate; +import org.hamcrest.BaseMatcher; +import org.hamcrest.Description; + +public class PredicateMatcher extends BaseMatcher { + + private final Class matchedClass; + private final Predicate predicate; + + public PredicateMatcher(Class matchedClass, Predicate predicate) { + this.matchedClass = matchedClass; + this.predicate = predicate; + } + + @Override + public boolean matches(Object item) { + if (matchedClass.isInstance(item)) + return predicate.test(matchedClass.cast(item)); + return false; + } + + @Override + public void describeTo(Description description) { + description.appendText("matches an item against a predicate"); + } +} From ed1cefa1443f90ea0a5b713fec8e9f38b8da31ee Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 6 Jun 2019 14:31:18 +0100 Subject: [PATCH 4/5] Use concurrent map for pending contact states. --- .../bramble/contact/ContactManagerImpl.java | 50 ++++++++-------- .../contact/ContactManagerImplTest.java | 58 +++++++++++-------- 2 files changed, 61 insertions(+), 47 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java index 17f8b49db..09047255f 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java @@ -33,13 +33,12 @@ import org.briarproject.bramble.api.transport.KeyManager; import java.security.GeneralSecurityException; import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.List; -import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.CopyOnWriteArrayList; import javax.annotation.Nullable; -import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; @@ -64,10 +63,8 @@ class ContactManagerImpl implements ContactManager, EventListener { private final EventBus eventBus; private final List hooks = new CopyOnWriteArrayList<>(); - private final Object statesLock = new Object(); - @GuardedBy("statesLock") - private final Map states = - new HashMap<>(); + private final ConcurrentMap states = + new ConcurrentHashMap<>(); @Inject ContactManagerImpl(DatabaseComponent db, @@ -176,10 +173,7 @@ class ContactManagerImpl implements ContactManager, EventListener { List> pairs = new ArrayList<>(pendingContacts.size()); for (PendingContact p : pendingContacts) { - PendingContactState state; - synchronized (statesLock) { - state = states.get(p.getId()); - } + PendingContactState state = states.get(p.getId()); if (state == null) state = WAITING_FOR_CONNECTION; pairs.add(new Pair<>(p, state)); } @@ -189,9 +183,7 @@ class ContactManagerImpl implements ContactManager, EventListener { @Override public void removePendingContact(PendingContactId p) throws DbException { db.transaction(false, txn -> db.removePendingContact(txn, p)); - synchronized (statesLock) { - states.remove(p); - } + states.remove(p); } @Override @@ -296,29 +288,39 @@ class ContactManagerImpl implements ContactManager, EventListener { if (e instanceof RendezvousConnectionOpenedEvent) { RendezvousConnectionOpenedEvent r = (RendezvousConnectionOpenedEvent) e; - setState(r.getPendingContactId(), ADDING_CONTACT); + PendingContactId p = r.getPendingContactId(); + setState(p, WAITING_FOR_CONNECTION, ADDING_CONTACT); } else if (e instanceof RendezvousConnectionClosedEvent) { RendezvousConnectionClosedEvent r = (RendezvousConnectionClosedEvent) e; // We're only interested in failures - if the rendezvous succeeds // the pending contact will be removed - if (!r.isSuccess()) - setState(r.getPendingContactId(), WAITING_FOR_CONNECTION); + if (!r.isSuccess()) { + PendingContactId p = r.getPendingContactId(); + setState(p, ADDING_CONTACT, WAITING_FOR_CONNECTION); + } } else if (e instanceof RendezvousFailedEvent) { RendezvousFailedEvent r = (RendezvousFailedEvent) e; setState(r.getPendingContactId(), FAILED); } } - /* - * Sets the state of the given pending contact and broadcasts an event, - * unless the current state is FAILED. + /** + * Sets the state of the given pending contact and broadcasts an event. */ private void setState(PendingContactId p, PendingContactState state) { - synchronized (statesLock) { - if (states.get(p) == FAILED) return; - states.put(p, state); + states.put(p, state); + eventBus.broadcast(new PendingContactStateChangedEvent(p, state)); + } + + /* + * Sets the state of the given pending contact and broadcasts an event if + * there is no current state or the current state equals {@code expected}. + */ + private void setState(PendingContactId p, PendingContactState expected, + PendingContactState state) { + PendingContactState old = states.putIfAbsent(p, state); + if (old == null || states.replace(p, expected, state)) eventBus.broadcast(new PendingContactStateChangedEvent(p, state)); - } } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java index 48928cefb..b569d7156 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java @@ -329,41 +329,53 @@ public class ContactManagerImplTest extends BrambleMockTestCase { } @Test - public void testFailedStateIsNotReplaced() throws Exception { - Transaction txn = new Transaction(null, true); - + public void testPendingContactFailsBeforeConnection() { + // The pending contact expires - the FAILED state is broadcast context.checking(new Expectations() {{ - oneOf(eventBus).broadcast(with(new PredicateMatcher<>( - PendingContactStateChangedEvent.class, e -> - e.getPendingContactState() == ADDING_CONTACT))); oneOf(eventBus).broadcast(with(new PredicateMatcher<>( PendingContactStateChangedEvent.class, e -> e.getPendingContactState() == FAILED))); }}); - - // A rendezvous connection is opened, then the pending contact expires, - // then the rendezvous connection is closed - contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( - pendingContact.getId())); contactManager.eventOccurred(new RendezvousFailedEvent( pendingContact.getId())); + + // A rendezvous connection is opened - no state is broadcast + contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( + pendingContact.getId())); + context.assertIsSatisfied(); + + // The rendezvous connection fails - no state is broadcast contactManager.eventOccurred(new RendezvousConnectionClosedEvent( pendingContact.getId(), false)); context.assertIsSatisfied(); + } - context.checking(new DbExpectations() {{ - oneOf(db).transactionWithResult(with(true), withDbCallable(txn)); - oneOf(db).getPendingContacts(txn); - will(returnValue(singletonList(pendingContact))); + @Test + public void testPendingContactFailsDuringConnection() { + // A rendezvous connection is opened - the ADDING_CONTACT state is + // broadcast + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == ADDING_CONTACT))); }}); - // The FAILED state should not have been overwritten - Collection> pairs = - contactManager.getPendingContacts(); - assertEquals(1, pairs.size()); - Pair pair = - pairs.iterator().next(); - assertEquals(pendingContact, pair.getFirst()); - assertEquals(FAILED, pair.getSecond()); + contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( + pendingContact.getId())); + context.assertIsSatisfied(); + + // The pending contact expires - the FAILED state is broadcast + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == FAILED))); + }}); + contactManager.eventOccurred(new RendezvousFailedEvent( + pendingContact.getId())); + + // The rendezvous connection fails - no state is broadcast + contactManager.eventOccurred(new RendezvousConnectionClosedEvent( + pendingContact.getId(), false)); + context.assertIsSatisfied(); } } From 64ae99bbceb6ab71080e70601363885d9f9fa4ce Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 6 Jun 2019 15:44:28 +0100 Subject: [PATCH 5/5] Handle corner cases such as removal during rendezvous. --- .../bramble/contact/ContactManagerImpl.java | 35 +++--- .../contact/ContactManagerImplTest.java | 101 ++++++++++++++++-- 2 files changed, 115 insertions(+), 21 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java index 09047255f..5c9ec6fa5 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/contact/ContactManagerImpl.java @@ -102,6 +102,7 @@ class ContactManagerImpl implements ContactManager, EventListener { throws DbException, GeneralSecurityException { PendingContact pendingContact = db.getPendingContact(txn, p); db.removePendingContact(txn, p); + states.remove(p); PublicKey theirPublicKey = pendingContact.getPublicKey(); ContactId c = db.addContact(txn, remote, local, theirPublicKey, verified); @@ -288,17 +289,13 @@ class ContactManagerImpl implements ContactManager, EventListener { if (e instanceof RendezvousConnectionOpenedEvent) { RendezvousConnectionOpenedEvent r = (RendezvousConnectionOpenedEvent) e; - PendingContactId p = r.getPendingContactId(); - setState(p, WAITING_FOR_CONNECTION, ADDING_CONTACT); + setStateConnected(r.getPendingContactId()); } else if (e instanceof RendezvousConnectionClosedEvent) { RendezvousConnectionClosedEvent r = (RendezvousConnectionClosedEvent) e; // We're only interested in failures - if the rendezvous succeeds // the pending contact will be removed - if (!r.isSuccess()) { - PendingContactId p = r.getPendingContactId(); - setState(p, ADDING_CONTACT, WAITING_FOR_CONNECTION); - } + if (!r.isSuccess()) setStateDisconnected(r.getPendingContactId()); } else if (e instanceof RendezvousFailedEvent) { RendezvousFailedEvent r = (RendezvousFailedEvent) e; setState(r.getPendingContactId(), FAILED); @@ -313,14 +310,22 @@ class ContactManagerImpl implements ContactManager, EventListener { eventBus.broadcast(new PendingContactStateChangedEvent(p, state)); } - /* - * Sets the state of the given pending contact and broadcasts an event if - * there is no current state or the current state equals {@code expected}. - */ - private void setState(PendingContactId p, PendingContactState expected, - PendingContactState state) { - PendingContactState old = states.putIfAbsent(p, state); - if (old == null || states.replace(p, expected, state)) - eventBus.broadcast(new PendingContactStateChangedEvent(p, state)); + private void setStateConnected(PendingContactId p) { + // Set the state to ADDING_CONTACT if there's no current state or the + // current state is WAITING_FOR_CONNECTION + if (states.putIfAbsent(p, ADDING_CONTACT) == null || + states.replace(p, WAITING_FOR_CONNECTION, ADDING_CONTACT)) { + eventBus.broadcast(new PendingContactStateChangedEvent(p, + ADDING_CONTACT)); + } + } + + private void setStateDisconnected(PendingContactId p) { + // Set the state to WAITING_FOR_CONNECTION if the current state is + // ADDING_CONTACT + if (states.replace(p, ADDING_CONTACT, WAITING_FOR_CONNECTION)) { + eventBus.broadcast(new PendingContactStateChangedEvent(p, + WAITING_FOR_CONNECTION)); + } } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java index b569d7156..b0a50ec16 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/contact/ContactManagerImplTest.java @@ -76,6 +76,9 @@ public class ContactManagerImplTest extends BrambleMockTestCase { private final KeyPair handshakeKeyPair = new KeyPair(getAgreementPublicKey(), getAgreementPrivateKey()); private final PendingContact pendingContact = getPendingContact(); + private final SecretKey rootKey = getSecretKey(); + private final long timestamp = System.currentTimeMillis(); + private final boolean alice = new Random().nextBoolean(); private ContactManagerImpl contactManager; @@ -87,9 +90,6 @@ public class ContactManagerImplTest extends BrambleMockTestCase { @Test public void testAddContact() throws Exception { - SecretKey rootKey = getSecretKey(); - long timestamp = System.currentTimeMillis(); - boolean alice = new Random().nextBoolean(); Transaction txn = new Transaction(null, false); context.checking(new DbExpectations() {{ @@ -329,7 +329,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { } @Test - public void testPendingContactFailsBeforeConnection() { + public void testPendingContactExpiresBeforeConnection() { // The pending contact expires - the FAILED state is broadcast context.checking(new Expectations() {{ oneOf(eventBus).broadcast(with(new PredicateMatcher<>( @@ -338,6 +338,7 @@ public class ContactManagerImplTest extends BrambleMockTestCase { }}); contactManager.eventOccurred(new RendezvousFailedEvent( pendingContact.getId())); + context.assertIsSatisfied(); // A rendezvous connection is opened - no state is broadcast contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( @@ -347,11 +348,10 @@ public class ContactManagerImplTest extends BrambleMockTestCase { // The rendezvous connection fails - no state is broadcast contactManager.eventOccurred(new RendezvousConnectionClosedEvent( pendingContact.getId(), false)); - context.assertIsSatisfied(); } @Test - public void testPendingContactFailsDuringConnection() { + public void testPendingContactExpiresDuringFailedConnection() { // A rendezvous connection is opened - the ADDING_CONTACT state is // broadcast context.checking(new Expectations() {{ @@ -370,12 +370,101 @@ public class ContactManagerImplTest extends BrambleMockTestCase { PendingContactStateChangedEvent.class, e -> e.getPendingContactState() == FAILED))); }}); + contactManager.eventOccurred(new RendezvousFailedEvent( pendingContact.getId())); + context.assertIsSatisfied(); // The rendezvous connection fails - no state is broadcast contactManager.eventOccurred(new RendezvousConnectionClosedEvent( pendingContact.getId(), false)); + } + + @Test + public void testPendingContactExpiresDuringSuccessfulConnection() + throws Exception { + Transaction txn = new Transaction(null, false); + + // A rendezvous connection is opened - the ADDING_CONTACT state is + // broadcast + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == ADDING_CONTACT))); + }}); + + contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( + pendingContact.getId())); context.assertIsSatisfied(); + + // The pending contact expires - the FAILED state is broadcast + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == FAILED))); + }}); + + contactManager.eventOccurred(new RendezvousFailedEvent( + pendingContact.getId())); + context.assertIsSatisfied(); + + // The pending contact is converted to a contact - no state is broadcast + context.checking(new DbExpectations() {{ + oneOf(db).getPendingContact(txn, pendingContact.getId()); + will(returnValue(pendingContact)); + oneOf(db).removePendingContact(txn, pendingContact.getId()); + oneOf(db).addContact(txn, remote, local, + pendingContact.getPublicKey(), verified); + will(returnValue(contactId)); + oneOf(db).setContactAlias(txn, contactId, + pendingContact.getAlias()); + oneOf(identityManager).getHandshakeKeys(txn); + will(returnValue(handshakeKeyPair)); + oneOf(keyManager).addContact(txn, contactId, + pendingContact.getPublicKey(), handshakeKeyPair); + oneOf(keyManager).addRotationKeys(txn, contactId, rootKey, + timestamp, alice, active); + oneOf(db).getContact(txn, contactId); + will(returnValue(contact)); + }}); + + contactManager.addContact(txn, pendingContact.getId(), remote, + local, rootKey, timestamp, alice, verified, active); + context.assertIsSatisfied(); + + // The rendezvous connection succeeds - no state is broadcast + contactManager.eventOccurred(new RendezvousConnectionClosedEvent( + pendingContact.getId(), true)); + } + + @Test + public void testPendingContactRemovedDuringFailedConnection() + throws Exception { + Transaction txn = new Transaction(null, false); + + // A rendezvous connection is opened - the ADDING_CONTACT state is + // broadcast + context.checking(new Expectations() {{ + oneOf(eventBus).broadcast(with(new PredicateMatcher<>( + PendingContactStateChangedEvent.class, e -> + e.getPendingContactState() == ADDING_CONTACT))); + }}); + + contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( + pendingContact.getId())); + context.assertIsSatisfied(); + + // The pending contact is removed - no state is broadcast + context.checking(new DbExpectations() {{ + oneOf(db).transaction(with(false), withDbRunnable(txn)); + oneOf(db).removePendingContact(txn, pendingContact.getId()); + }}); + + contactManager.removePendingContact(pendingContact.getId()); + context.assertIsSatisfied(); + + // The rendezvous connection fails - no state is broadcast + contactManager.eventOccurred(new RendezvousConnectionClosedEvent( + pendingContact.getId(), false)); } }