Skip to content

Replace tokio mutex with std mutex #1866

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

Closed

Conversation

CodeMan62
Copy link
Contributor

Motivation

  1. Performance: std::sync::Mutex is generally more efficient when the critical sections are short and contention is low, as it avoids the overhead of async machinery.
  2. Simplicity: Standard mutex provides a simpler implementation that's more straightforward to reason about.
  3. Best Practices: As noted in the Tokio documentation, it's recommended to use std::sync::Mutex when:
    • The critical sections are short
    • The mutex isn't held across .await points
    • Performance is a priority

Fixes #1062

Solution

The solution involves:

  1. Replacing instances of tokio::sync::Mutex with std::sync::Mutex throughout the codebase
  2. Updating the corresponding lock/unlock patterns to use the standard library implementation
  3. Ensuring that no mutex is held across .await points to maintain correct async behavior
  4. Verifying that all critical sections are short-lived to maximize performance benefits

Thanks for reviewing it let me know if any changes required

@CodeMan62 CodeMan62 requested a review from a team as a code owner April 4, 2025 19:49
@@ -53,12 +53,12 @@ impl ReceiverOutput for QueueForwarder {
}

async fn handle(&self, request: ForwardRequest) -> Result<(), BoxError> {
let mut sender = self.sender.lock().await;
let mut sender = self.sender.lock().unwrap();
Copy link
Member

@tasn tasn Apr 4, 2025

Choose a reason for hiding this comment

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

This seems wrong? Won't be block the async thread every time we wait on a lock?

Also the unwrap feels like a code smell.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the sender is by necessity held over await points so we must not use a std Mutex for it.

Copy link
Member

@tasn tasn left a comment

Choose a reason for hiding this comment

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

@CodeMan62 CodeMan62 force-pushed the replace-tokio-mutex-with-std-mutex branch from acf5518 to 97c16f8 Compare April 6, 2025 06:07
@CodeMan62 CodeMan62 force-pushed the replace-tokio-mutex-with-std-mutex branch from b9f986c to 16fc322 Compare April 6, 2025 06:57
Copy link
Member

@svix-jplatte svix-jplatte left a comment

Choose a reason for hiding this comment

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

All of the changes outside of the test code still look wrong to me. I'm pretty sure we need the async mutex in all of those cases. And for the test code, it doesn't seem all that useful to switch either.

I will trigger CI to run again to make sure, but unless you have a specific reason to believe there is a worthwhile change here, I'd encourage you to close this PR and I'll close the associated issue since it doesn't look like there was anything of substance there really.

@CodeMan62
Copy link
Contributor Author

associated

Hii @svix-jplatte thanks for review i am closing this PR. please close the associated issue too.

@CodeMan62 CodeMan62 closed this Apr 9, 2025
@CodeMan62 CodeMan62 deleted the replace-tokio-mutex-with-std-mutex branch April 24, 2025 05:21
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.

Replace tokio mutex with std mutex
3 participants