Fix potential deadlock in the table manager #5472
Conversation
| } | ||
|
|
||
| table = NewTable(tableName, filepath.Join(tm.cfg.CacheDir, tableName), tm.indexStorageClient, tm.boltIndexClient, tm.metrics) | ||
| tm.tables[tableName] = table |
There was a problem hiding this comment.
don't you need to write lock here for this ?
There was a problem hiding this comment.
I missed that we're using RLock at the top. I've changed it to Lock
| tm.tablesMtx.RLock() | ||
| defer tm.tablesMtx.RUnlock() |
There was a problem hiding this comment.
I think we're fixing a deadlock and introducing a new one. We use RLock first as a lower-cost way to check if the table exists, but must RUnlock it after (the previous PR did this). We cannot defer it here because we need to Lock it later if the table wasn't found. I think we can introduce a 1 line fix:defer tm.tablesMtx.Unlock() immediately after the write lock is acquired, which will ensure we release the write lock correctly in all cases it's acquired.
There was a problem hiding this comment.
Yeah, I missed the Rlock vs. Lock
The latest commit just defers tm.tablesMtx.Unlock() after it's created.
| }) | ||
|
|
||
| t.Run("it doesn't deadlock when table create fails", func(t *testing.T) { | ||
| tempDir := os.TempDir() |
There was a problem hiding this comment.
Just a nit, lets use t.TempDir like other places.
There was a problem hiding this comment.
Sorry, I pushed this change and resolved the merge conflict because I wanted to cut a new release with this fix.
…patterson/fix-tablemanager-deadlock'
I was looking at how the tablemanager worked and noticed that
getOrCreateTablehad a deadlock. I've included a test that illustrates the issue and fixed it.