From 4712e20049ebc24c4902049c47aadac27ffdc549 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Mon, 1 Jul 2024 21:50:30 +0900 Subject: [PATCH 1/4] ffldb: throw error when attempting to delete an open file This change lets us test that we don't attempt to delete open files with unit tests. On Windows this behavior is not allowed which results in an error but on linux and osx it's allowed. To better test Windows compatibilty adding this explicit check in is useful. --- database/ffldb/blockio.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/database/ffldb/blockio.go b/database/ffldb/blockio.go index 2b415a17b0..2272ba1658 100644 --- a/database/ffldb/blockio.go +++ b/database/ffldb/blockio.go @@ -307,6 +307,11 @@ func (s *blockStore) openFile(fileNum uint32) (*lockableFile, error) { // other state cleanup necessary. func (s *blockStore) deleteFile(fileNum uint32) error { filePath := blockFilePath(s.basePath, fileNum) + blockFile := s.openBlockFiles[fileNum] + if blockFile != nil { + err := fmt.Errorf("attempted to delete open file at %v", filePath) + return makeDbErr(database.ErrDriverSpecific, err.Error(), err) + } if err := os.Remove(filePath); err != nil { return makeDbErr(database.ErrDriverSpecific, err.Error(), err) } From 8b5f2aa6f2cbd6c73d51d3f6c7251c7f48288175 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Mon, 1 Jul 2024 22:01:36 +0900 Subject: [PATCH 2/4] ffldb: add check for deleting files that are open This check let's us ensure that attempting to delete open files are caught during unit tests. --- database/ffldb/driver_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/database/ffldb/driver_test.go b/database/ffldb/driver_test.go index 0b2f452032..e48491a12d 100644 --- a/database/ffldb/driver_test.go +++ b/database/ffldb/driver_test.go @@ -335,6 +335,20 @@ func TestPrune(t *testing.T) { t.Fatal(err) } + // Open the first block file before the pruning happens in the + // code snippet below. This let's us test that block files are + // properly closed before attempting to delete them. + err = db.View(func(tx database.Tx) error { + _, err := tx.FetchBlock(blocks[0].Hash()) + if err != nil { + return err + } + return nil + }) + if err != nil { + t.Fatal(err) + } + var deletedBlocks []chainhash.Hash // This should leave 3 files on disk. From 8ed8ef134067e8af529996c4d027913b5945bf13 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Mon, 1 Jul 2024 22:10:42 +0900 Subject: [PATCH 3/4] ffldb: refactor out file close code into its own method The existing file close code is refactored out into it's own method closeFile() so that it can be reused elsewhere. --- database/ffldb/blockio.go | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/database/ffldb/blockio.go b/database/ffldb/blockio.go index 2272ba1658..77f97f592e 100644 --- a/database/ffldb/blockio.go +++ b/database/ffldb/blockio.go @@ -281,17 +281,7 @@ func (s *blockStore) openFile(fileNum uint32) (*lockableFile, error) { lruList := s.openBlocksLRU if lruList.Len() >= maxOpenFiles { lruFileNum := lruList.Remove(lruList.Back()).(uint32) - oldBlockFile := s.openBlockFiles[lruFileNum] - - // Close the old file under the write lock for the file in case - // any readers are currently reading from it so it's not closed - // out from under them. - oldBlockFile.Lock() - _ = oldBlockFile.file.Close() - oldBlockFile.Unlock() - - delete(s.openBlockFiles, lruFileNum) - delete(s.fileNumToLRUElem, lruFileNum) + s.closeFile(lruFileNum) } s.fileNumToLRUElem[fileNum] = lruList.PushFront(fileNum) s.lruMutex.Unlock() @@ -302,6 +292,26 @@ func (s *blockStore) openFile(fileNum uint32) (*lockableFile, error) { return blockFile, nil } +// closeFile checks that the file corresponding to the file number is open and +// if it is, it closes it in a concurrency safe manner and cleans up associated +// data in the blockstore struct. +func (s *blockStore) closeFile(fileNum uint32) { + blockFile := s.openBlockFiles[fileNum] + if blockFile == nil { + return + } + + // Close the old file under the write lock for the file in case + // any readers are currently reading from it so it's not closed + // out from under them. + blockFile.Lock() + _ = blockFile.file.Close() + blockFile.Unlock() + + delete(s.openBlockFiles, fileNum) + delete(s.fileNumToLRUElem, fileNum) +} + // deleteFile removes the block file for the passed flat file number. The file // must already be closed and it is the responsibility of the caller to do any // other state cleanup necessary. From c9fae1ac7cca6e6d55baec913286e483e28923a9 Mon Sep 17 00:00:00 2001 From: Calvin Kim Date: Mon, 1 Jul 2024 20:15:18 +0900 Subject: [PATCH 4/4] ffldb: close block files before deleting them The block files may be open when deleteFile is called. This resulted in files not being deleted and erroring out on windows. Properly closing the files closing the files avoids this error. --- database/ffldb/db.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/database/ffldb/db.go b/database/ffldb/db.go index 3e96bfc738..60103aaa58 100644 --- a/database/ffldb/db.go +++ b/database/ffldb/db.go @@ -1630,6 +1630,9 @@ func (tx *transaction) writePendingAndCommit() error { // We do this first before doing any of the writes as we can't undo // deletions of files. for _, fileNum := range tx.pendingDelFileNums { + // Make sure the file is closed before attempting to delete it. + tx.db.store.closeFile(fileNum) + err := tx.db.store.deleteFileFunc(fileNum) if err != nil { // Nothing we can do if we fail to delete blocks besides