-
Notifications
You must be signed in to change notification settings - Fork 982
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
Add shared mutable state documentation #1759
Add shared mutable state documentation #1759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't feel this is appropriate to have in axum's docs 🤷
Maybe we should link to tokio's "Which kind of mutex should you use?" docs though? |
Yeah that'd be fine with me. |
@ThanePatrol check the referenced pull request I crested #1777. |
@Vagelis-Prokopiou Thanks for letting me know. I've shared the repo with you and incorporated your changes. |
The documentation for sharing a mutable state did not provide examples. Wrote explanation of the steps required to share mutable states between routes. Provided an example in code. This was inspired from discussion tokio-rs#629 tokio-rs#629
The cost of using an async mutex was not mentioned. Rather than defaulting to async, the documentation now gives the std Mutex in the example and links to the discussion from tokio.
Example was opinionated and specific. Also, changed written description to be more general.
Provides more consistent style across code base
Vagelis had a similar pull request. Changes were merged. Added a note about deadlocks. A detailed discussion may not be appropriate. However, a note that makes people aware of the potential consequences could help reduce pitfalls of share mutable state.
e95ba1b
to
7ebccbb
Compare
This PR looks ready. Kindly waiting on the owners @davidpdrsn @jplatte. |
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@jplatte suggested change applied. |
I had lots of nitpicky suggestions so I've rewritten it myself. @jplatte wanna give it a final look? |
/// #[derive(Clone)] | ||
/// struct AppState { | ||
/// data: Arc<Mutex<String>>, | ||
/// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried that people will do fine-grained locking where more coarse locking is the right solution, based on this example. I'm not sure just showing an Arc<Mutex<_>>
with everything inside locked at once is better either though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jplatte Maybe something like the following can be added, in order to make your point super explicit?
#[derive(Clone)]
struct AppState {
// This is just an example usage, not a best practice.
// Choose a setup that matches your specific use case.
data: Arc<Mutex<String>>,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried that people will do fine-grained locking where more coarse locking is the right solution, based on this example.
Yeah I'm a little worried about that as well. But like you say putting a mutex around the whole state probably isn't right either 😅 I dunno. Think I'll just keep it as is.
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Motivation
I spent a lot of time trying to find documentation for creating a mutable state. I eventually found discussion #629
Solution
Added documentation for creating mutable state and wrote a small trivial example using a
Vec<String>