Better thread safety pattern and some tests

model/message.go

@@ -76,15 +76,15 @@ func (t *Timeline) Less(i, j int) bool {
 // Insert inserts a message into the timeline in a thread safe way.
 func (t *Timeline) Insert(mi *Message) {
 	t.lock.Lock()
+	defer t.lock.Unlock()
 
 	for _, m := range t.Messages {
+		// If the message already exists, then we don't add it
 		if compareSignatures(m.Signature, mi.Signature) {
-			t.lock.Unlock()
 			return
 		}
 	}
 
 	t.Messages = append(t.Messages, *mi)
 	sort.Sort(t)
-	t.lock.Unlock()
 }

model/message_test.go

@@ -59,7 +59,7 @@ func TestTranscriptConsistency(t *testing.T) {
 
 	t.Logf("group: %v, sarah %v", group, sarah)
 
-	c1, _ := sarah.EncryptMessageToGroup("Hello World 1", group.GroupID)
+	c1, _ := alice.EncryptMessageToGroup("Hello World 1", group.GroupID)
 	t.Logf("Length of Encrypted Message: %v", len(c1))
 	alice.AttemptDecryption(c1)
 
@@ -81,6 +81,7 @@ func TestTranscriptConsistency(t *testing.T) {
 	t.Logf("Length of Encrypted Message: %v", len(c5))
 
 	_, m1 := sarah.AttemptDecryption(c1)
+	sarah.AttemptDecryption(c1) // Try a duplicate
 	_, m2 := sarah.AttemptDecryption(c2)
 	_, m3 := sarah.AttemptDecryption(c3)
 	_, m4 := sarah.AttemptDecryption(c4)
@@ -93,7 +94,7 @@ func TestTranscriptConsistency(t *testing.T) {
 	timeline.Insert(m3)
 	timeline.Insert(m2)
 
-	for i, m := range timeline.GetMessages() {
+	for i, m := range group.GetTimeline() {
 		if m.Message != "Hello World "+strconv.Itoa(i+1) {
 			t.Fatalf("Timeline Out of Order!: %v %v", i, m)
 		}

model/profile.go

@@ -93,45 +93,42 @@ func (p *Profile) RejectInvite(groupID string) {
 }
 
 // AcceptInvite accepts a group invite
-func (p *Profile) AcceptInvite(groupID string) error {
+func (p *Profile) AcceptInvite(groupID string) (err error) {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	group, ok := p.Groups[groupID]
 	if ok {
 		group.Accepted = true
+	} else {
+		err = errors.New("group does not exist")
 	}
-	p.lock.Unlock()
+	return
-	if !ok {
-		return errors.New("group does not exist")
-	}
-	return nil
 }
 
 // BlockPeer blocks a contact
-func (p *Profile) BlockPeer(onion string) error {
+func (p *Profile) BlockPeer(onion string) (err error) {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	contact, ok := p.Contacts[onion]
 	if ok {
 		contact.Blocked = true
+	} else {
+		err = errors.New("peer does not exist")
 	}
-	p.lock.Unlock()
+	return
-	if !ok {
-		return errors.New("peer does not exist")
-	}
-	return nil
 }
 
 // TrustPeer sets a contact to trusted
-func (p *Profile) TrustPeer(onion string) error {
+func (p *Profile) TrustPeer(onion string) (err error) {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	contact, ok := p.Contacts[onion]
 	if ok {
 		contact.Trusted = true
+	} else {
+		err = errors.New("peer does not exist")
 	}
-	p.lock.Unlock()
+	return
-	if !ok {
-		return errors.New("peer does not exist")
-	}
-	return nil
 }
 
 // IsBlocked returns true if the contact has been blocked, false otherwise
@@ -145,8 +142,8 @@ func (p *Profile) IsBlocked(onion string) bool {
 
 func (p *Profile) GetContact(onion string) (*PublicProfile, bool) {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	contact, ok := p.Contacts[onion]
-	p.lock.Unlock()
 	return contact, ok
 }
 
@@ -181,17 +178,17 @@ func (p *Profile) StartGroup(server string) (groupID string, invite []byte, err
 	group.SignGroup(signedGroupID)
 	invite, err = group.Invite()
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	p.Groups[group.GroupID] = group
-	p.lock.Unlock()
 	return
 }
 
 // GetGroupByGroupID a pointer to a Group by the group Id, returns nil if no group found.
 func (p *Profile) GetGroupByGroupID(groupID string) (g *Group) {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	g = p.Groups[groupID]
-	p.lock.Unlock()
+	return
-	return g
 }
 
 // ProcessInvite adds a new group invite to the profile.
@@ -208,23 +205,21 @@ func (p *Profile) ProcessInvite(gci *protocol.GroupChatInvite, peerHostname stri
 
 // AddGroup is a convenience method for adding a group to a profile.
 func (p *Profile) AddGroup(group *Group) {
-	p.lock.Lock()
 	existingGroup, exists := p.Groups[group.GroupID]
-	p.lock.Unlock()
 	if !exists {
 		owner, ok := p.GetContact(group.Owner)
 		if ok {
 			valid := ed25519.Verify(owner.Ed25519PublicKey, []byte(group.GroupID+group.GroupServer), group.SignedGroupID)
 			if valid {
 				p.lock.Lock()
+				defer p.lock.Unlock()
 				p.Groups[group.GroupID] = group
-				p.lock.Unlock()
 			}
 		}
 	} else if exists && existingGroup.Owner == group.Owner {
 		p.lock.Lock()
+		defer p.lock.Unlock()
 		p.Groups[group.GroupID] = group
-		p.lock.Unlock()
 	}
 
 	// If we are sent an invite or group update by someone who is not an owner
@@ -300,8 +295,8 @@ func (p *Profile) EncryptMessageToGroup(message string, groupID string) ([]byte,
 // Save makes a opy of the profile in the given file
 func (p *Profile) Save(profilefile string) error {
 	p.lock.Lock()
+	defer p.lock.Unlock()
 	bytes, _ := json.Marshal(p)
-	p.lock.Unlock()
 	return ioutil.WriteFile(profilefile, bytes, 0600)
 }
 

model/profile_test.go

@@ -37,6 +37,65 @@ func TestProfileIdentity(t *testing.T) {
 	t.Logf("%v", alice)
 }
 
+func TestTrustPeer(t *testing.T) {
+	sarah := GenerateNewProfile("Sarah")
+	alice := GenerateNewProfile("Alice")
+	sarah.AddContact(alice.Onion, &alice.PublicProfile)
+	alice.AddContact(sarah.Onion, &sarah.PublicProfile)
+	alice.TrustPeer(sarah.Onion)
+	if alice.IsBlocked(sarah.Onion) {
+		t.Errorf("peer should not be blocked")
+	}
+
+	if alice.TrustPeer("") == nil {
+		t.Errorf("trusting a non existent peer should error")
+	}
+}
+
+func TestBlockPeer(t *testing.T) {
+	sarah := GenerateNewProfile("Sarah")
+	alice := GenerateNewProfile("Alice")
+	sarah.AddContact(alice.Onion, &alice.PublicProfile)
+	alice.AddContact(sarah.Onion, &sarah.PublicProfile)
+	alice.BlockPeer(sarah.Onion)
+	if !alice.IsBlocked(sarah.Onion) {
+		t.Errorf("peer should not be blocked")
+	}
+
+	if alice.BlockPeer("") == nil {
+		t.Errorf("blocking a non existent peer should error")
+	}
+}
+
+func TestAcceptNonExistentGroup(t *testing.T) {
+	sarah := GenerateNewProfile("Sarah")
+	sarah.AcceptInvite("doesnotexist")
+}
+
+func TestRejectGroupInvite(t *testing.T) {
+	sarah := GenerateNewProfile("Sarah")
+	alice := GenerateNewProfile("Alice")
+	sarah.AddContact(alice.Onion, &alice.PublicProfile)
+	alice.AddContact(sarah.Onion, &sarah.PublicProfile)
+
+	gid, invite, _ := alice.StartGroup("aaa.onion")
+	gci := &protocol.CwtchPeerPacket{}
+	proto.Unmarshal(invite, gci)
+	sarah.ProcessInvite(gci.GetGroupChatInvite(), alice.Onion)
+	group := alice.GetGroupByGroupID(gid)
+	if len(sarah.Groups) == 1 {
+		if sarah.GetGroupByGroupID(group.GroupID).Accepted {
+			t.Errorf("Group should not be accepted")
+		}
+		sarah.RejectInvite(group.GroupID)
+		if len(sarah.Groups) != 0 {
+			t.Errorf("Group %v should have been deleted", group.GroupID)
+		}
+		return
+	}
+	t.Errorf("Group should exist in map")
+}
+
 func TestProfileGroup(t *testing.T) {
 	sarah := GenerateNewProfile("Sarah")
 	alice := GenerateNewProfile("Alice")
@@ -47,8 +106,8 @@ func TestProfileGroup(t *testing.T) {
 	gci := &protocol.CwtchPeerPacket{}
 	proto.Unmarshal(invite, gci)
 	sarah.ProcessInvite(gci.GetGroupChatInvite(), alice.Onion)
-
 	group := alice.GetGroupByGroupID(gid)
+	sarah.AcceptInvite(group.GroupID)
 	c, _ := sarah.EncryptMessageToGroup("Hello World", group.GroupID)
 	alice.AttemptDecryption(c)
 

storage/message_store.go

@@ -25,8 +25,10 @@ type MessageStore struct {
 
 // Close closes the message store and underlying resources.
 func (ms *MessageStore) Close() {
+	ms.lock.Lock()
 	ms.messages = nil
 	ms.file.Close()
+	ms.lock.Unlock()
 }
 
 // Init sets up a MessageStore backed by filename