From ed1cefa1443f90ea0a5b713fec8e9f38b8da31ee Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 6 Jun 2019 14:31:18 +0100 Subject: [PATCH] Use concurrent map for pending contact states. --- .../bramble/contact/ContactManagerImpl.java | 50 +++++++++-------- .../contact/ContactManagerImplTest.java | 56 +++++++++++-------- 2 files changed, 60 insertions(+), 46 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))); }}); + contactManager.eventOccurred(new RendezvousFailedEvent( + pendingContact.getId())); - // A rendezvous connection is opened, then the pending contact expires, - // then the rendezvous connection is closed + // A rendezvous connection is opened - no state is broadcast contactManager.eventOccurred(new RendezvousConnectionOpenedEvent( pendingContact.getId())); - contactManager.eventOccurred(new RendezvousFailedEvent( - 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(); } } -- 2.24.1