Race condition with SQLite3Store.startCleanup

[earthboundkid]

My tests hit a race. IIUC, the cause of the race is that there is a bug in SQLite3Store.

func NewWithCleanupInterval(db *sql.DB, cleanupInterval time.Duration) *SQLite3Store {
	p := &SQLite3Store{db: db}
	if cleanupInterval > 0 {
		go p.startCleanup(cleanupInterval)
	}
	return p
}

Notice p.startCleanup(cleanupInterval) is called with go.

func (p *SQLite3Store) startCleanup(interval time.Duration) {
	p.stopCleanup = make(chan bool)
	...

Observe that p.stopCleanup is written to inside that goroutine, without any lock on p.

Finally

func (p *SQLite3Store) StopCleanup() {
	if p.stopCleanup != nil {
		p.stopCleanup <- true
	}
}

The line p.stopCleanup <- true is good and race free, but p.stopCleanup != nil is a race because it is reading stopCleanup, which was written to in the startCleanup routine.

I think the solution is to change NewWithCleanupInterval to

func NewWithCleanupInterval(db *sql.DB, cleanupInterval time.Duration) *SQLite3Store {
	p := &SQLite3Store{db: db}
	if cleanupInterval > 0 {
		p.stopCleanup = make(chan bool) // NEW!
		go p.startCleanup(cleanupInterval)
	}
	return p
}

And remove p.stopCleanup = make(chan bool) from startCleanup.

[earthboundkid]

Here is a test that fails under -race:

diff --git a/sqlite3store/sqlite3store_test.go b/sqlite3store/sqlite3store_test.go
index 71908a5..dd809f1 100644
--- a/sqlite3store/sqlite3store_test.go
+++ b/sqlite3store/sqlite3store_test.go
@@ -269,7 +269,6 @@ func TestCleanup(t *testing.T) {
 	}
 
 	p := NewWithCleanupInterval(db, 200*time.Millisecond)
-	defer p.StopCleanup()
 
 	err = p.Commit("session_token", []byte("encoded_data"), time.Now().Add(100*time.Millisecond))
 	if err != nil {
@@ -287,6 +286,8 @@ func TestCleanup(t *testing.T) {
 	}
 
 	time.Sleep(300 * time.Millisecond)
+	p.StopCleanup()
+
 	row = db.QueryRow("SELECT COUNT(*) FROM sessions WHERE token = 'session_token'")
 	err = row.Scan(&count)
 	if err != nil {

$ go test -race -v -run TestCleanup .
=== RUN   TestCleanup
==================
WARNING: DATA RACE
Read at 0x00c00011e5c8 by goroutine 7:
  github.com/alexedwards/scs/sqlite3store.(*SQLite3Store).StopCleanup()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store.go:126 +0x4f8
  github.com/alexedwards/scs/sqlite3store.TestCleanup()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store_test.go:289 +0x4f4
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1743 +0x40

Previous write at 0x00c00011e5c8 by goroutine 9:
  github.com/alexedwards/scs/sqlite3store.(*SQLite3Store).startCleanup()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store.go:99 +0x54
  github.com/alexedwards/scs/sqlite3store.NewWithCleanupInterval.gowrap1()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store.go:28 +0x40

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1743 +0x5e0
  testing.runTests.func1()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:2168 +0x80
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1690 +0x184
  testing.runTests()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:2166 +0x6e0
  testing.(*M).Run()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:2034 +0xb74
  main.main()
      _testmain.go:57 +0x110

Goroutine 9 (running) created at:
  github.com/alexedwards/scs/sqlite3store.NewWithCleanupInterval()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store.go:28 +0x128
  github.com/alexedwards/scs/sqlite3store.TestCleanup()
      /Users/cjohnson/src/gowork/scs/sqlite3store/sqlite3store_test.go:271 +0x284
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/Cellar/go/1.23.2/libexec/src/testing/testing.go:1743 +0x40
==================
    testing.go:1399: race detected during execution of test
--- FAIL: TestCleanup (0.31s)
FAIL
FAIL	github.com/alexedwards/scs/sqlite3store	0.534s
FAIL

The above change fixes the race:

diff --git a/sqlite3store/sqlite3store.go b/sqlite3store/sqlite3store.go
index ff2860e..7b05ff0 100644
--- a/sqlite3store/sqlite3store.go
+++ b/sqlite3store/sqlite3store.go
@@ -25,6 +25,7 @@ func New(db *sql.DB) *SQLite3Store {
 func NewWithCleanupInterval(db *sql.DB, cleanupInterval time.Duration) *SQLite3Store {
 	p := &SQLite3Store{db: db}
 	if cleanupInterval > 0 {
+		p.stopCleanup = make(chan bool)
 		go p.startCleanup(cleanupInterval)
 	}
 	return p
@@ -96,7 +97,6 @@ func (p *SQLite3Store) All() (map[string][]byte, error) {
 }
 
 func (p *SQLite3Store) startCleanup(interval time.Duration) {
-	p.stopCleanup = make(chan bool)
 	ticker := time.NewTicker(interval)
 	for {
 		select {