Merge branch '900-remove-error-state' into 'master'

Remove error state and reset session on error instead

Closes #900

See merge request !484

briar-core/src/main/java/org/briarproject/briar/sharing/BlogProtocolEngineImpl.java

@@ -14,7 +14,6 @@ import org.briarproject.bramble.api.system.Clock;
 import org.briarproject.briar.api.blog.Blog;
 import org.briarproject.briar.api.blog.BlogInvitationResponse;
 import org.briarproject.briar.api.blog.BlogManager;
-import org.briarproject.briar.api.blog.BlogSharingManager;
 import org.briarproject.briar.api.blog.event.BlogInvitationRequestReceivedEvent;
 import org.briarproject.briar.api.blog.event.BlogInvitationResponseReceivedEvent;
 import org.briarproject.briar.api.client.MessageTracker;
@@ -74,8 +73,8 @@ class BlogProtocolEngineImpl extends ProtocolEngineImpl<Blog> {
 	}
 
 	@Override
-	protected ClientId getClientId() {
-		return BlogSharingManager.CLIENT_ID;
+	protected ClientId getShareableClientId() {
+		return BlogManager.CLIENT_ID;
 	}
 
 	@Override

briar-core/src/main/java/org/briarproject/briar/sharing/ForumProtocolEngineImpl.java

@@ -15,7 +15,6 @@ import org.briarproject.briar.api.client.MessageTracker;
 import org.briarproject.briar.api.forum.Forum;
 import org.briarproject.briar.api.forum.ForumInvitationResponse;
 import org.briarproject.briar.api.forum.ForumManager;
-import org.briarproject.briar.api.forum.ForumSharingManager;
 import org.briarproject.briar.api.forum.event.ForumInvitationRequestReceivedEvent;
 import org.briarproject.briar.api.forum.event.ForumInvitationResponseReceivedEvent;
 import org.briarproject.briar.api.sharing.InvitationRequest;
@@ -74,8 +73,8 @@ class ForumProtocolEngineImpl extends ProtocolEngineImpl<Forum> {
 	}
 
 	@Override
-	protected ClientId getClientId() {
-		return ForumSharingManager.CLIENT_ID;
+	protected ClientId getShareableClientId() {
+		return ForumManager.CLIENT_ID;
 	}
 
 	@Override

briar-core/src/main/java/org/briarproject/briar/sharing/ProtocolEngineImpl.java

@@ -36,7 +36,6 @@ import static org.briarproject.briar.sharing.MessageType.DECLINE;
 import static org.briarproject.briar.sharing.MessageType.INVITE;
 import static org.briarproject.briar.sharing.MessageType.LEAVE;
 import static org.briarproject.briar.sharing.SharingConstants.GROUP_KEY_CONTACT_ID;
-import static org.briarproject.briar.sharing.State.ERROR;
 import static org.briarproject.briar.sharing.State.LOCAL_INVITED;
 import static org.briarproject.briar.sharing.State.LOCAL_LEFT;
 import static org.briarproject.briar.sharing.State.REMOTE_HANGING;
@@ -79,7 +78,6 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case SHARING:
 			case LOCAL_LEFT:
 			case REMOTE_HANGING:
-			case ERROR:
 				throw new ProtocolStateException(); // Invalid in these states
 			default:
 				throw new AssertionError();
@@ -132,7 +130,6 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case SHARING:
 			case LOCAL_LEFT:
 			case REMOTE_HANGING:
-			case ERROR:
 				throw new ProtocolStateException(); // Invalid in these states
 			default:
 				throw new AssertionError();
@@ -188,7 +185,6 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case SHARING:
 			case LOCAL_LEFT:
 			case REMOTE_HANGING:
-			case ERROR:
 				throw new ProtocolStateException(); // Invalid in these states
 			default:
 				throw new AssertionError();
@@ -232,7 +228,6 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case LOCAL_INVITED:
 			case LOCAL_LEFT:
 			case REMOTE_HANGING:
-			case ERROR:
 				return s; // Ignored in this state
 			default:
 				throw new AssertionError();
@@ -277,9 +272,7 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 				return onRemoteInvite(txn, s, m, false, LOCAL_LEFT);
 			case LOCAL_INVITED:
 			case SHARING:
-				return abort(txn, s); // Invalid in these states
-			case ERROR:
-				return s; // Ignored in this state
+				return abortWithMessage(txn, s); // Invalid in these states
 			default:
 				throw new AssertionError();
 		}
@@ -289,10 +282,11 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			InviteMessage<S> m, boolean available, State nextState)
 			throws DbException, FormatException {
 		// The timestamp must be higher than the last invite message, if any
-		if (m.getTimestamp() <= s.getInviteTimestamp()) return abort(txn, s);
+		if (m.getTimestamp() <= s.getInviteTimestamp())
+			return abortWithMessage(txn, s);
 		// The dependency, if any, must be the last remote message
 		if (!isValidDependency(s, m.getPreviousMessageId()))
-			return abort(txn, s);
+			return abortWithMessage(txn, s);
 		// Mark the invite message visible in the UI and (un)available to answer
 		markMessageVisibleInUi(txn, m.getId(), true);
 		markMessageAvailableToAnswer(txn, m.getId(), available);
@@ -312,10 +306,11 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 	private Session onRemoteInviteWhenInvited(Transaction txn, Session s,
 			InviteMessage<S> m) throws DbException, FormatException {
 		// The timestamp must be higher than the last invite message, if any
-		if (m.getTimestamp() <= s.getInviteTimestamp()) return abort(txn, s);
+		if (m.getTimestamp() <= s.getInviteTimestamp())
+			return abortWithMessage(txn, s);
 		// The dependency, if any, must be the last remote message
 		if (!isValidDependency(s, m.getPreviousMessageId()))
-			return abort(txn, s);
+			return abortWithMessage(txn, s);
 		// Mark the invite message visible in the UI and unavailable to answer
 		markMessageVisibleInUi(txn, m.getId(), true);
 		markMessageAvailableToAnswer(txn, m.getId(), false);
@@ -349,9 +344,7 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case LOCAL_INVITED:
 			case SHARING:
 			case LOCAL_LEFT:
-				return abort(txn, s); // Invalid in these states
-			case ERROR:
-				return s; // Ignored in this state
+				return abortWithMessage(txn, s); // Invalid in these states
 			default:
 				throw new AssertionError();
 		}
@@ -360,10 +353,11 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 	private Session onRemoteAccept(Transaction txn, Session s, AcceptMessage m,
 			State nextState) throws DbException, FormatException {
 		// The timestamp must be higher than the last invite message
-		if (m.getTimestamp() <= s.getInviteTimestamp()) return abort(txn, s);
+		if (m.getTimestamp() <= s.getInviteTimestamp())
+			return abortWithMessage(txn, s);
 		// The dependency, if any, must be the last remote message
 		if (!isValidDependency(s, m.getPreviousMessageId()))
-			return abort(txn, s);
+			return abortWithMessage(txn, s);
 		// Mark the response visible in the UI
 		markMessageVisibleInUi(txn, m.getId(), true);
 		// Track the message
@@ -382,8 +376,8 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			AcceptMessage m) throws DbException, FormatException {
 		// Perform normal remote accept validation and operation
 		Session session = onRemoteAccept(txn, s, m, SHARING);
-		// Share the shareable with the contact
-		if (session.getState() != ERROR)
+		// Share the shareable with the contact, if session was not reset
+		if (session.getState() != START)
 			setShareableVisibility(txn, s, SHARED);
 		return session;
 	}
@@ -402,9 +396,7 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case LOCAL_INVITED:
 			case SHARING:
 			case LOCAL_LEFT:
-				return abort(txn, s); // Invalid in these states
-			case ERROR:
-				return s; // Ignored in this state
+				return abortWithMessage(txn, s); // Invalid in these states
 			default:
 				throw new AssertionError();
 		}
@@ -413,10 +405,11 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 	private Session onRemoteDecline(Transaction txn, Session s,
 			DeclineMessage m) throws DbException, FormatException {
 		// The timestamp must be higher than the last invite message
-		if (m.getTimestamp() <= s.getInviteTimestamp()) return abort(txn, s);
+		if (m.getTimestamp() <= s.getInviteTimestamp())
+			return abortWithMessage(txn, s);
 		// The dependency, if any, must be the last remote message
 		if (!isValidDependency(s, m.getPreviousMessageId()))
-			return abort(txn, s);
+			return abortWithMessage(txn, s);
 		// Mark the response visible in the UI
 		markMessageVisibleInUi(txn, m.getId(), true);
 		// Track the message
@@ -447,15 +440,13 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			case LOCAL_INVITED:
 				return onRemoteLeaveWhenInvited(txn, s, m);
 			case LOCAL_LEFT:
-				return onRemoteLeave(txn, s, m);
+				return onRemoteLeaveWhenLocalLeft(txn, s, m);
 			case SHARING:
 				return onRemoteLeaveWhenSharing(txn, s, m);
 			case START:
 			case REMOTE_INVITED:
 			case REMOTE_HANGING:
-				return abort(txn, s); // Invalid in these states
-			case ERROR:
-				return s; // Ignored in this state
+				return abortWithMessage(txn, s); // Invalid in these states
 			default:
 				throw new AssertionError();
 		}
@@ -463,28 +454,22 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 
 	private Session onRemoteLeaveWhenInvited(Transaction txn, Session s,
 			LeaveMessage m) throws DbException, FormatException {
-		// Carry out normal leave validation and operation
-		Session session = onRemoteLeave(txn, s, m);
+		// The dependency, if any, must be the last remote message
+		if (!isValidDependency(s, m.getPreviousMessageId()))
+			return abortWithMessage(txn, s);
 		// Mark any invite messages in the session unavailable to answer
-		if (session.getState() != ERROR)
-			markInvitesUnavailableToAnswer(txn, s);
+		markInvitesUnavailableToAnswer(txn, s);
 		// Move to the next state
-		return session;
+		return new Session(START, s.getContactGroupId(), s.getShareableId(),
+				s.getLastLocalMessageId(), m.getId(), s.getLocalTimestamp(),
+				s.getInviteTimestamp());
 	}
 
-	private Session onRemoteLeave(Transaction txn, Session s, LeaveMessage m)
-			throws DbException, FormatException {
+	private Session onRemoteLeaveWhenLocalLeft(Transaction txn, Session s,
+			LeaveMessage m) throws DbException, FormatException {
 		// The dependency, if any, must be the last remote message
 		if (!isValidDependency(s, m.getPreviousMessageId()))
-			return abort(txn, s);
-		if (s.getState() == SHARING) {
-			// Broadcast event informing that contact left
-			ContactId contactId = getContactId(txn, s.getContactGroupId());
-			ContactLeftShareableEvent e =
-					new ContactLeftShareableEvent(s.getShareableId(),
-							contactId);
-			txn.attach(e);
-		}
+			return abortWithMessage(txn, s);
 		// Move to the next state
 		return new Session(START, s.getContactGroupId(), s.getShareableId(),
 				s.getLastLocalMessageId(), m.getId(), s.getLocalTimestamp(),
@@ -493,36 +478,48 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 
 	private Session onRemoteLeaveWhenSharing(Transaction txn, Session s,
 			LeaveMessage m) throws DbException, FormatException {
-		// Carry out normal leave validation and operation
-		Session session = onRemoteLeave(txn, s, m);
+		// The dependency, if any, must be the last remote message
+		if (!isValidDependency(s, m.getPreviousMessageId()))
+			return abortWithMessage(txn, s);
+		// Broadcast event informing that contact left
+		ContactId contactId = getContactId(txn, s.getContactGroupId());
+		ContactLeftShareableEvent e =
+				new ContactLeftShareableEvent(s.getShareableId(),
+						contactId);
+		txn.attach(e);
 		// Stop sharing the shareable with the contact
-		if (session.getState() != ERROR)
-			setShareableVisibility(txn, s, INVISIBLE);
+		setShareableVisibility(txn, s, INVISIBLE);
 		// Move to the next state
-		return session;
+		return new Session(START, s.getContactGroupId(), s.getShareableId(),
+				s.getLastLocalMessageId(), m.getId(), s.getLocalTimestamp(),
+				s.getInviteTimestamp());
 	}
 
 	@Override
 	public Session onAbortMessage(Transaction txn, Session s, AbortMessage m)
 			throws DbException, FormatException {
-		return abort(txn, s);
+		abort(txn, s);
+		return new Session(START, s.getContactGroupId(), s.getShareableId(),
+				null, m.getId(), 0, 0);
 	}
 
-	private Session abort(Transaction txn, Session s)
+	private void abort(Transaction txn, Session s)
 			throws DbException, FormatException {
-		// If the session has already been aborted, do nothing
-		if (s.getState() == ERROR) return s;
 		// Mark any invite messages in the session unavailable to answer
 		markInvitesUnavailableToAnswer(txn, s);
 		// If we subscribe, make the shareable invisible to the contact
 		if (isSubscribed(txn, s.getShareableId()))
 			setShareableVisibility(txn, s, INVISIBLE);
+	}
+
+	private Session abortWithMessage(Transaction txn, Session s)
+			throws DbException, FormatException {
+		abort(txn, s);
 		// Send an ABORT message
 		Message sent = sendAbortMessage(txn, s);
-		// Move to the ERROR state
-		return new Session(ERROR, s.getContactGroupId(), s.getShareableId(),
-				sent.getId(), s.getLastRemoteMessageId(), sent.getTimestamp(),
-				s.getInviteTimestamp());
+		// Reset the session back to initial state
+		return new Session(START, s.getContactGroupId(), s.getShareableId(),
+				sent.getId(), null, 0, 0);
 	}
 
 	private void markInvitesUnavailableToAnswer(Transaction txn, Session s)
@@ -541,10 +538,10 @@ abstract class ProtocolEngineImpl<S extends Shareable>
 			throws DbException {
 		if (!db.containsGroup(txn, g)) return false;
 		Group group = db.getGroup(txn, g);
-		return group.getClientId().equals(getClientId());
+		return group.getClientId().equals(getShareableClientId());
 	}
 
-	protected abstract ClientId getClientId();
+	protected abstract ClientId getShareableClientId();
 
 	private Message sendAbortMessage(Transaction txn, Session session)
 			throws DbException {

briar-core/src/main/java/org/briarproject/briar/sharing/State.java

@@ -10,7 +10,7 @@ import javax.annotation.concurrent.Immutable;
 enum State {
 
 	START(0), LOCAL_INVITED(1), REMOTE_INVITED(2), SHARING(3), LOCAL_LEFT(4),
-	REMOTE_HANGING(5), ERROR(6);
+	REMOTE_HANGING(5);
 
 	private final int value;
 

briar-core/src/test/java/org/briarproject/briar/sharing/ForumSharingIntegrationTest.java

@@ -3,11 +3,14 @@ package org.briarproject.briar.sharing;
 import net.jodah.concurrentunit.Waiter;
 
 import org.briarproject.bramble.api.contact.Contact;
+import org.briarproject.bramble.api.data.BdfDictionary;
 import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.db.Transaction;
 import org.briarproject.bramble.api.event.Event;
 import org.briarproject.bramble.api.event.EventListener;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
+import org.briarproject.bramble.api.sync.Message;
+import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.bramble.test.TestDatabaseModule;
 import org.briarproject.briar.api.forum.Forum;
 import org.briarproject.briar.api.forum.ForumInvitationRequest;
@@ -42,6 +45,7 @@ public class ForumSharingIntegrationTest
 		extends BriarIntegrationTest<BriarIntegrationTestComponent> {
 
 	private ForumManager forumManager0, forumManager1;
+	private MessageEncoder messageEncoder;
 	private SharerListener listener0, listener2;
 	private InviteeListener listener1;
 	private Forum forum0;
@@ -67,6 +71,7 @@ public class ForumSharingIntegrationTest
 		forumSharingManager0 = c0.getForumSharingManager();
 		forumSharingManager1 = c1.getForumSharingManager();
 		forumSharingManager2 = c2.getForumSharingManager();
+		messageEncoder = new MessageEncoderImpl(clientHelper, messageFactory);
 
 		// initialize waiter fresh for each test
 		eventWaiter = new Waiter();
@@ -686,6 +691,74 @@ public class ForumSharingIntegrationTest
 		assertTrue(found);
 	}
 
+	@Test
+	public void testSessionResetAfterAbort() throws Exception {
+		// initialize and let invitee accept all requests
+		listenToEvents(true);
+
+		// send invitation
+		forumSharingManager0
+				.sendInvitation(forum0.getId(), contactId1From0, "Hi!",
+						clock.currentTimeMillis());
+
+		// sync first request message
+		sync0To1(1, true);
+		eventWaiter.await(TIMEOUT, 1);
+		assertTrue(listener1.requestReceived);
+
+		// get invitation MessageId for later
+		MessageId invitationId = null;
+		for (InvitationMessage m : forumSharingManager1
+				.getInvitationMessages(contactId0From1)) {
+			if (m instanceof ForumInvitationRequest) {
+				invitationId = m.getId();
+			}
+		}
+		assertNotNull(invitationId);
+
+		// sync response back
+		sync1To0(1, true);
+		eventWaiter.await(TIMEOUT, 1);
+		assertTrue(listener0.responseReceived);
+
+		// forum is shared mutually
+		assertTrue(forumSharingManager0.getSharedWith(forum0.getId())
+				.contains(contact1From0));
+		assertTrue(forumSharingManager1.getSharedWith(forum0.getId())
+				.contains(contact0From1));
+
+		// send an accept message for the same forum
+		Message m = messageEncoder.encodeAcceptMessage(
+				forumSharingManager0.getContactGroup(contact1From0).getId(),
+				forum0.getId(), clock.currentTimeMillis(), invitationId);
+		c0.getClientHelper().addLocalMessage(m, new BdfDictionary(), true);
+
+		// sync unexpected message and the expected abort message back
+		sync0To1(1, true);
+		sync1To0(1, true);
+
+		// forum is no longer shared mutually
+		assertFalse(forumSharingManager0.getSharedWith(forum0.getId())
+				.contains(contact1From0));
+		assertFalse(forumSharingManager1.getSharedWith(forum0.getId())
+				.contains(contact0From1));
+
+		// new invitation is possible now
+		forumSharingManager0
+				.sendInvitation(forum0.getId(), contactId1From0, null,
+						clock.currentTimeMillis());
+		sync0To1(1, true);
+
+		// and can be answered
+		sync1To0(1, true);
+
+		// forum is shared mutually again
+		assertTrue(forumSharingManager0.getSharedWith(forum0.getId())
+				.contains(contact1From0));
+		assertTrue(forumSharingManager1.getSharedWith(forum0.getId())
+				.contains(contact0From1));
+	}
+
 	@NotNullByDefault
 	private class SharerListener implements EventListener {
 

briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTest.java

@@ -18,6 +18,7 @@ import org.briarproject.bramble.api.identity.LocalAuthor;
 import org.briarproject.bramble.api.lifecycle.LifecycleManager;
 import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault;
 import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault;
+import org.briarproject.bramble.api.sync.MessageFactory;
 import org.briarproject.bramble.api.sync.SyncSession;
 import org.briarproject.bramble.api.sync.SyncSessionFactory;
 import org.briarproject.bramble.api.sync.event.MessageStateChangedEvent;
@@ -100,6 +101,8 @@ public abstract class BriarIntegrationTest<C extends BriarIntegrationTestCompone
 	@Inject
 	protected AuthorFactory authorFactory;
 	@Inject
+	protected MessageFactory messageFactory;
+	@Inject
 	protected ContactGroupFactory contactGroupFactory;
 	@Inject
 	protected PrivateGroupFactory privateGroupFactory;

briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTestComponent.java

@@ -25,7 +25,6 @@ import org.briarproject.bramble.test.TestSeedProviderModule;
 import org.briarproject.bramble.transport.TransportModule;
 import org.briarproject.briar.api.blog.BlogManager;
 import org.briarproject.briar.api.blog.BlogSharingManager;
-import org.briarproject.briar.api.client.MessageQueueManager;
 import org.briarproject.briar.api.client.MessageTracker;
 import org.briarproject.briar.api.forum.ForumManager;
 import org.briarproject.briar.api.forum.ForumSharingManager;
@@ -133,8 +132,6 @@ public interface BriarIntegrationTestComponent {
 
 	MessageTracker getMessageTracker();
 
-	MessageQueueManager getMessageQueueManager();
-
 	PrivateGroupManager getPrivateGroupManager();
 
 	TransportPropertyManager getTransportPropertyManager();