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

Prevent a deadlock in expiration #22374

Merged
merged 4 commits into from Aug 16, 2023
Merged

Prevent a deadlock in expiration #22374

merged 4 commits into from Aug 16, 2023

Conversation

ncabatoff
Copy link
Collaborator

Deadlock occurs when Restore grabs lease lock then pending lock, while a revocationJob's OnFailure grabs those lock in the reverse order.

…ding lock, while a revocationJob's OnFailure grabs those lock in the reverse order.
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 16, 2023
@github-actions
Copy link

github-actions bot commented Aug 16, 2023

CI Results:
Failures:

Test Type Package Test Logs
builtin/logical/pkiext/pkiext_binary Test_ACME view test results
builtin/logical/pkiext/pkiext_binary Test_ACME/group view test results
builtin/logical/pkiext/pkiext_binary Test_ACME/group/caddy_http_eab view test results

@ncabatoff ncabatoff marked this pull request as ready for review August 16, 2023 18:47
@ncabatoff ncabatoff added this to the 1.12.10 milestone Aug 16, 2023
@ncabatoff ncabatoff added backport/1.12.x backport/1.13.x Backport changes to `release/1.13.x` backport/1.14.x Backport changes to `release/1.14.x` labels Aug 16, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@jasonodonnell jasonodonnell self-requested a review August 16, 2023 19:28
Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

I had a look through this code and I agree with this fix. This should prevent the pending lock being held while the individual lease's lock is being grabbed. In particular, this should prevent a deadlock when a lease is being expired and restored at the same time, as far as I understand it.

There's a chance we haven't gotten to the bottom of this, but based on my understanding, this should either fix the problem or make it much better.

@ncabatoff ncabatoff merged commit abaf1d6 into main Aug 16, 2023
97 of 99 checks passed
@ncabatoff ncabatoff deleted the fix-lease-restore-deadlock branch August 16, 2023 20:04
rebwill pushed a commit that referenced this pull request Aug 18, 2023
* Prevent a deadlock that occurs when Restore grabs lease lock then pending lock, while a revocationJob's OnFailure grabs those lock in the reverse order.

* Fix lock

* Add CL

* Grab lock before markLeaseIrrevocable

---------

Co-authored-by: hc-github-team-secure-vault-core <github-team-secure-vault-core@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13.x Backport changes to `release/1.13.x` backport/1.14.x Backport changes to `release/1.14.x` hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants