Address review comments

briar-api/src/org/briarproject/api/clients/MessageQueueManager.java

@@ -1,5 +1,6 @@
 package org.briarproject.api.clients;
 
+import org.briarproject.api.FormatException;
 import org.briarproject.api.db.DbException;
 import org.briarproject.api.db.Metadata;
 import org.briarproject.api.db.Transaction;
@@ -49,16 +50,19 @@ public interface MessageQueueManager {
 		 * Called once for each incoming message that passes validation.
 		 * Messages are passed to the hook in order.
 		 *
-		 * @throws DbException should only be used for real database errors
+		 * @throws DbException Should only be used for real database errors.
-		 * @throws InvalidMessageException for any non-database error
+		 * If this is thrown, delivery will be attempted again at next startup,
+		 * whereas if an FormatException is thrown,
+		 * the message will be permanently invalidated.
+		 * @throws FormatException for any non-database error
 		 * that occurs while handling remotely created data.
 		 * This includes errors that occur while handling locally created data
 		 * in a context controlled by remotely created data
 		 * (for example, parsing the metadata of a dependency
 		 * of an incoming message).
-		 * Never rethrow DbException as InvalidMessageException
+		 * Never rethrow DbException as FormatException!
 		 */
 		void incomingMessage(Transaction txn, QueueMessage q, Metadata meta)
-				throws DbException, InvalidMessageException;
+				throws DbException, FormatException;
 	}
 }

briar-api/src/org/briarproject/api/sync/ValidationManager.java

@@ -57,7 +57,10 @@ public interface ValidationManager {
 		 * Called once for each incoming message that passes validation.
 		 *
 		 * @return whether or not this message should be shared
-		 * @throws DbException should only be used for real database errors
+		 * @throws DbException Should only be used for real database errors.
+		 * If this is thrown, delivery will be attempted again at next startup,
+		 * whereas if an InvalidMessageException is thrown,
+		 * the message will be permanently invalidated.
 		 * @throws InvalidMessageException for any non-database error
 		 * that occurs while handling remotely created data.
 		 * This includes errors that occur while handling locally created data
@@ -66,7 +69,7 @@ public interface ValidationManager {
 		 * of an incoming message).
 		 * Throwing this will delete the incoming message and its metadata
 		 * marking it as invalid in the database.
-		 * Never rethrow DbException as InvalidMessageException
+		 * Never rethrow DbException as InvalidMessageException!
 		 */
 		boolean incomingMessage(Transaction txn, Message m, Metadata meta)
 				throws DbException, InvalidMessageException;

briar-core/src/org/briarproject/clients/BdfIncomingMessageHook.java

@@ -62,26 +62,26 @@ public abstract class BdfIncomingMessageHook implements IncomingMessageHook,
 	@Override
 	public boolean incomingMessage(Transaction txn, Message m, Metadata meta)
 			throws DbException, InvalidMessageException {
-		return incomingMessage(txn, m, meta, MESSAGE_HEADER_LENGTH);
+		try {
+			return incomingMessage(txn, m, meta, MESSAGE_HEADER_LENGTH);
+		} catch (FormatException e) {
+			throw new InvalidMessageException(e);
+		}
 	}
 
 	@Override
 	public void incomingMessage(Transaction txn, QueueMessage q, Metadata meta)
-			throws DbException, InvalidMessageException {
+			throws DbException, FormatException {
 		incomingMessage(txn, q, meta, QUEUE_MESSAGE_HEADER_LENGTH);
 	}
 
 	private boolean incomingMessage(Transaction txn, Message m, Metadata meta,
-			int headerLength) throws DbException, InvalidMessageException {
+			int headerLength) throws DbException, FormatException {
-		try {
+		byte[] raw = m.getRaw();
-			byte[] raw = m.getRaw();
+		BdfList body = clientHelper.toList(raw, headerLength,
-			BdfList body = clientHelper.toList(raw, headerLength,
+				raw.length - headerLength);
-					raw.length - headerLength);
+		BdfDictionary metaDictionary = metadataParser.parse(meta);
-			BdfDictionary metaDictionary = metadataParser.parse(meta);
+		return incomingMessage(txn, m, body, metaDictionary);
-			return incomingMessage(txn, m, body, metaDictionary);
-		} catch (FormatException e) {
-			throw new InvalidMessageException(e);
-		}
 	}
 
 	protected void trackIncomingMessage(Transaction txn, Message m)

briar-core/src/org/briarproject/clients/MessageQueueManagerImpl.java

@@ -227,16 +227,20 @@ class MessageQueueManagerImpl implements MessageQueueManager {
 				// Save the queue state before passing control to the delegate
 				saveQueueState(txn, m.getGroupId(), queueState);
 				// Deliver the messages to the delegate
-				delegate.incomingMessage(txn, q, meta);
+				try {
-				for (MessageId id : consecutive) {
-					byte[] raw = db.getRawMessage(txn, id);
-					meta = db.getMessageMetadata(txn, id);
-					q = queueMessageFactory.createMessage(id, raw);
-					if (LOG.isLoggable(INFO)) {
-						LOG.info("Delivering pending message with position "
-								+ q.getQueuePosition());
-					}
 					delegate.incomingMessage(txn, q, meta);
+					for (MessageId id : consecutive) {
+						byte[] raw = db.getRawMessage(txn, id);
+						meta = db.getMessageMetadata(txn, id);
+						q = queueMessageFactory.createMessage(id, raw);
+						if (LOG.isLoggable(INFO)) {
+							LOG.info("Delivering pending message with position "
+									+ q.getQueuePosition());
+						}
+						delegate.incomingMessage(txn, q, meta);
+					}
+				} catch (FormatException e) {
+					throw new InvalidMessageException(e);
 				}
 			}
 			// message queues are only useful for groups with two members

briar-core/src/org/briarproject/introduction/IntroductionManagerImpl.java

@@ -246,7 +246,9 @@ class IntroductionManagerImpl extends ConversationClientImpl
 				} else if (role == ROLE_INTRODUCEE) {
 					introduceeManager.incomingMessage(txn, state, message);
 				} else {
-					throw new AssertionError("Unknown role '" + role + "'");
+					if (LOG.isLoggable(WARNING))
+						LOG.warning("Unknown role '" + role + "'");
+					throw new DbException();
 				}
 				if (type == TYPE_RESPONSE) trackIncomingMessage(txn, m);
 			} catch (DbException e) {

briar-tests/src/org/briarproject/sync/ValidationManagerImplTest.java

@@ -110,8 +110,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the first message
 			oneOf(hook).incomingMessage(txn2, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn2, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn2, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn2, messageId);
@@ -215,8 +213,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn2, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn2, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn2, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn2, messageId);
@@ -240,8 +236,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the dependent
 			oneOf(hook).incomingMessage(txn3, message2, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn3, messageId2);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn3, messageId2, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn3, messageId2);
@@ -366,8 +360,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn1, message, metadata);
 			will(returnValue(true));
-			oneOf(db).getRawMessage(txn1, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn1, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn1, messageId);
@@ -589,8 +581,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn1, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn1, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn1, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn1, messageId);
@@ -707,8 +697,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn1, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn1, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn1, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn1, messageId);
@@ -953,8 +941,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn1, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn1, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn1, messageId, DELIVERED);
 			// The message has two pending dependents: 1 and 2
 			oneOf(db).getMessageDependents(txn1, messageId);
@@ -978,8 +964,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver message 1
 			oneOf(hook).incomingMessage(txn2, message1, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn2, messageId1);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn2, messageId1, DELIVERED);
 			// Message 1 has one pending dependent: 3
 			oneOf(db).getMessageDependents(txn2, messageId1);
@@ -1003,8 +987,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver message 2
 			oneOf(hook).incomingMessage(txn3, message2, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn3, messageId2);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn3, messageId2, DELIVERED);
 			// Message 2 has one pending dependent: 3 (same dependent as 1)
 			oneOf(db).getMessageDependents(txn3, messageId2);
@@ -1027,8 +1009,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			will(returnValue(metadata));
 			// Deliver message 3
 			oneOf(hook).incomingMessage(txn4, message3, metadata);
-			oneOf(db).getRawMessage(txn4, messageId3);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn4, messageId3, DELIVERED);
 			// Message 3 has one pending dependent: 4
 			oneOf(db).getMessageDependents(txn4, messageId3);
@@ -1059,8 +1039,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver message 4
 			oneOf(hook).incomingMessage(txn6, message4, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn6, messageId4);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn6, messageId4, DELIVERED);
 			// Message 4 has no pending dependents
 			oneOf(db).getMessageDependents(txn6, messageId4);
@@ -1111,8 +1089,6 @@ public class ValidationManagerImplTest extends BriarTestCase {
 			// Deliver the message
 			oneOf(hook).incomingMessage(txn1, message, metadata);
 			will(returnValue(false));
-			oneOf(db).getRawMessage(txn1, messageId);
-			will(returnValue(raw));
 			oneOf(db).setMessageState(txn1, messageId, DELIVERED);
 			// Get any pending dependents
 			oneOf(db).getMessageDependents(txn1, messageId);