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

Reduce critical section in rate limiter code #365

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

MartinquaXD
Copy link
Contributor

@MartinquaXD MartinquaXD commented Jul 14, 2022

While investigating a deadlock which started happening recently [0] I noticed that matching on a temporary value extends that temporaries lifetime to the duration of the match statement. It can be extended even further when you return the match expression as a temporary value itself.
This is usually not a problem but can lead to surprising deadlocks when the temporary value contains a MutexGuard or something similar [1] [2]. Then the resource gets locked longer than expected.

I originally wrote the rate limiter code with the intention of NOT locking self.strategy for the entire duration of the log statement so it makes sense to fix that now.

However, I'm not convinced that this bug caused the investigated deadlock. The code always acquired the locks in following order 1. self.strategy 2. stdout and should have released it in the reverse order. For this lifetime extension to cause the deadlock there would also have to be a critical section which first locks stdout then self.strategy which I don't think it does anywhere.

Test Plan

existing tests

@MartinquaXD MartinquaXD requested a review from a team as a code owner July 14, 2022 07:49
Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

The unexpected behavior around mutex guard temporaries is a good find and I like the change to make the behavior of the code easier to understand.
I also think that is not the cause for the deadlock because locking stdout is likely completely internal to the tracing functions.

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM!

@MartinquaXD MartinquaXD enabled auto-merge (squash) July 27, 2022 10:47
@MartinquaXD MartinquaXD merged commit b0820d2 into main Jul 27, 2022
@MartinquaXD MartinquaXD deleted the reduce-criticial-section-in-rate-limiter branch July 27, 2022 10:47
@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants