Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ffldb: close block files #2208

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

kcalvinalvin
Copy link
Collaborator

On methods readBlock and readBlockRegion, the opened files were never closed which resulted in errors when attempting to delete these files on pruning on windows. Properly closing the files avoids this error.

Doing a full block sync on windows to make sure. Definitely seems to be the fix as I'm not having any issues so far (few hours into ibd).

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9743641160

Details

  • 2 of 12 (16.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 57.228%

Changes Missing Coverage Covered Lines Changed/Added Lines %
database/ffldb/db.go 2 12 16.67%
Totals Coverage Status
Change from base Build 9701102084: 0.03%
Covered Lines: 29839
Relevant Lines: 52141

💛 - Coveralls

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.
This check let's us ensure that attempting to delete open files are
caught during unit tests.
The existing file close code is refactored out into it's own method
closeFile() so that it can be reused elsewhere.
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.
@kcalvinalvin kcalvinalvin marked this pull request as ready for review July 1, 2024 13:16
@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9744922494

Details

  • 17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 57.202%

Changes Missing Coverage Covered Lines Changed/Added Lines %
database/ffldb/blockio.go 14 17 82.35%
Totals Coverage Status
Change from base Build 9701102084: 0.002%
Covered Lines: 29824
Relevant Lines: 52138

💛 - Coveralls

@Roasbeef
Copy link
Member

Roasbeef commented Jul 1, 2024

he opened files were never closed which resulted in errors when attempting to delete these files on pruning on windows.

I've had similar issues with Windows, basically that you always need to close files before doing operations such as rename or delete.

@kcalvinalvin
Copy link
Collaborator Author

he opened files were never closed which resulted in errors when attempting to delete these files on pruning on windows.

I've had similar issues with Windows, basically that you always need to close files before doing operations such as rename or delete.

Yeah that seems to have been the issue. No troubles syncing with pruning turned on

@saubyk
Copy link

saubyk commented Jul 9, 2024

cc: @guggero for review

@guggero guggero self-requested a review July 9, 2024 16:37
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎎

@Roasbeef Roasbeef merged commit 2134387 into btcsuite:master Jul 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants