From 0b82e0f83fc211c84f0b899bd5c778ba9c677688 Mon Sep 17 00:00:00 2001
From: Josh Deprez <DrJosh9000@users.noreply.github.com>
Date: Thu, 18 Jun 2020 11:52:35 +1000
Subject: [PATCH] Improvements from review

- remove a TODO
- inline newSyncSession (only reference)
- defer the unlock for readability
---
 engine/sync.go      | 23 +++++++++--------------
 engine/sync_test.go |  2 --
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/engine/sync.go b/engine/sync.go
index 085f156..2ad2746 100644
--- a/engine/sync.go
+++ b/engine/sync.go
@@ -206,18 +206,6 @@ type syncSession struct {
 	notes chan *SyncNote
 }
 
-// newSyncSession returns an initialised synchronisation session.
-func newSyncSession(node net.IP, id SyncSessionID) *syncSession {
-	return &syncSession{
-		id:         id,
-		node:       node,
-		desync:     true,
-		startTime:  time.Now(),
-		expiryTime: time.Now().Add(sessionDeadtime),
-		notes:      make(chan *SyncNote, sessionNotesQueueSize),
-	}
-}
-
 // addNote adds a notification to the synchronisation session. If the notes
 // channel is full the session is marked as desynchronised and the notification
 // is discarded.
@@ -258,10 +246,17 @@ func newSyncServer(e *Engine) *syncServer {
 // the provided node.
 func (s *syncServer) newSession(node net.IP) *syncSession {
 	s.sessionLock.Lock()
-	session := newSyncSession(node, s.nextSessionID)
+	defer s.sessionLock.Unlock()
+	session := &syncSession{
+		id:         s.nextSessionID,
+		node:       node,
+		desync:     true,
+		startTime:  time.Now(),
+		expiryTime: time.Now().Add(sessionDeadtime),
+		notes:      make(chan *SyncNote, sessionNotesQueueSize),
+	}
 	s.nextSessionID++
 	s.sessions[session.id] = session
-	s.sessionLock.Unlock()
 
 	return session
 }
diff --git a/engine/sync_test.go b/engine/sync_test.go
index 478fe2e..1041de1 100644
--- a/engine/sync_test.go
+++ b/engine/sync_test.go
@@ -125,8 +125,6 @@ func TestSyncHeartbeats(t *testing.T) {
 	go server.run()
 	go client.run()
 
-	// TODO: Test we can disable and re-enable the client
-
 	// Enabling briefly shouldn't have time to get a heartbeat, but should get
 	// the initial desync.
 	client.enable()