Move to async #31
Move to async #31
Conversation
|
what needs to happen for this to go upstream? |
|
Hey @mkawalec - thanks for putting this PR together. A lot of things needed to be updated to accomplish the move to async but I think this represents a very strong body of work. I have a few suggestions for this
I would like to spend some more time doing further review for the more complicated changes presented |
| @@ -21,7 +23,7 @@ struct DemoDTOPartial { | |||
| an_int: u32, | |||
| } | |||
|
|
|||
| fn write_document(session: &mut ServiceSession, doc_id: &str) -> errors::Result<WriteResult> { | |||
| async fn write_document(session: &mut ServiceSession, doc_id: &str) -> errors::Result<WriteResult> { | |||
There was a problem hiding this comment.
Generally the question to ask here in the examples is if we want to convert all of the examples to async code. I don't think the lib should imply that you must use async (though I think most people would indeed chose to do so).
Personally I would choose to move forward with the async approach through these examples. Curious if you have any thoughts @davidgraeff ?
| @@ -42,7 +55,7 @@ pub struct Credentials { | |||
| pub client_id: String, | |||
| pub api_key: String, | |||
| #[serde(default, skip)] | |||
| pub(crate) keys: Keys, | |||
| pub(crate) keys: Arc<RwLock<Keys>>, | |||
There was a problem hiding this comment.
Out of curiousity - what is the motivation for Arc<RwLock> on Keys?
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "firestore-db-and-auth" | |||
| version = "0.6.1" | |||
| version = "0.8.0" | |||
There was a problem hiding this comment.
I would argue this should probably be a major version breaking change. With the changes proposed here all sync code disappears - meaning this library becomes exclusively async. Changes here would no longer be compatible with those using the sync only version of this library
|
Thanks a lot for the contribution, thanks @JadedEvan for the review. Async support has been merged with #34. I have taken your pull request as base and added the missing parts. This crate is async only now. |
I needed to use this library from an async context, decided to move it all the way to async-only after experimenting with having sync/async behind feature flags.
Let me know what you think about this approach @davidgraeff, the tests and Rocket-specific code still need to be ported to async.
When downloading google JWKS, I parse the
Cache-Controlheader to know how long the keys are valid for and download new JWKS if the keys have expired. We are using this library with long running server processes and Google tends to rotate these keys every so often causing rare token validation failures.asyncis infectious so it leaks out here and there, but I was mostly guided by the speed of getting this port out of the door to unblock some async contexts in our codebase.