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

fix: add start_time to ProcessUID on StorageLock #8753

Merged
merged 11 commits into from
Jun 12, 2024
Merged

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Jun 11, 2024

Makes the identifier more unique by adding start_time.

Fixes the following kind of issue:

  1. reth node with PID 9 locks storage.
  2. reth node stops non-gracefully leaving the lockfiles behind.
  3. firefox starts with PID 9.
  4. reth node attempts to start but realizes that PID 9 is executing, and thus, does not acquire the lock.

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-db Related to the database A-static-files Related to static files labels Jun 11, 2024
@joshieDo joshieDo requested a review from rakita as a code owner June 11, 2024 15:01
crates/storage/db/src/lockfile.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think this should work

@joshieDo joshieDo enabled auto-merge June 11, 2024 15:24
@joshieDo joshieDo disabled auto-merge June 11, 2024 15:53
@joshieDo
Copy link
Collaborator Author

joshieDo commented Jun 11, 2024

Calling sysinfo::System to get the process start_time is too expensive during ef-tests. So, added the feature disable_lock, which will make StorageLock::try_acquire always succeed, and use it during ef-tests.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

pedantic suggestions

crates/storage/db/src/lockfile.rs Outdated Show resolved Hide resolved
crates/storage/db/Cargo.toml Outdated Show resolved Hide resolved
@@ -78,6 +78,7 @@ mdbx = ["reth-libmdbx"]
bench = []
arbitrary = ["reth-primitives/arbitrary", "reth-db-api/arbitrary"]
optimism = []
disable_lock = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this feature enabled do we then need sysinfo dep?

opting out is a lot harder than opting in, but in this case we expect that this is only disabled for testing, so imo this is fine

@joshieDo joshieDo added this pull request to the merge queue Jun 12, 2024
Merged via the queue into main with commit c265038 Jun 12, 2024
29 checks passed
@joshieDo joshieDo deleted the joshie/lock-start-time branch June 12, 2024 09:40
emhane pushed a commit that referenced this pull request Jun 13, 2024
@Thegaram Thegaram mentioned this pull request Sep 27, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database A-static-files Related to static files C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants