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(migration): release storage locks #4347

Merged
merged 2 commits into from
May 7, 2024

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented May 7, 2024

Describe your changes

Closes #4344. Hat tip to @erwanor for spotting the omission.

Issue ticket number and link

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Code change fixes migration behavior, so is very relevant to not breaking consensus.

Testing/review

This change is a bit tricky to test manually, but the rationale in #4344 is sound. I'd like to get it in, so we can rebase #4337 on top of it, and go back to debugging #4340.

Closes #4344. Hat tip to @erwanor for spotting the omission.
We're seeing an error related to polling the proving keys via the proto
package's build.rs. Let's just enable lfs=true on the git checkout
action.
@hdevalence
Copy link
Member

Non-blocking, but, wouldn't dropping the Storage be a more RAII-like way to release the lock?

Copy link
Member

@zbuc zbuc left a comment

Choose a reason for hiding this comment

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

LGTM though I'm a little uncomfortable with the pattern of needing to manually manage storage locks in migrations

@erwanor
Copy link
Member

erwanor commented May 7, 2024

@hdevalence we can't await aborting tasks using RAII, the problem is that releasing resources happens after the handle has been dropped

@conorsch conorsch merged commit b3e835a into main May 7, 2024
13 checks passed
@conorsch conorsch deleted the 4344-migration-logic-release-storage-locks branch May 7, 2024 19:59
@cratelyn cratelyn added this to the Sprint 6 milestone May 8, 2024
@cratelyn cratelyn added C-bug Category: a bug migration A pull request that contains a migration labels May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: a bug migration A pull request that contains a migration
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

storage: possible race in migration logic
5 participants