-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make KeyringProvider::fetch_* async #3089
Conversation
crates/uv-auth/src/middleware.rs
Outdated
@@ -142,6 +142,7 @@ impl Middleware for AuthMiddleware { | |||
.and_then(|credentials| credentials.username()) | |||
{ | |||
debug!("Checking keyring for credentials for {url}"); | |||
// how to fix this | |||
keyring.fetch(request.url(), username) |
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.
hi @zanieb I googled for a while, but still no idea how to edit this. It reports await
can't be called from a closure.
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 believe the idiomatic way to do it is with match
instead
diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs
index 191ca92e..451f690a 100644
--- a/crates/uv-auth/src/middleware.rs
+++ b/crates/uv-auth/src/middleware.rs
@@ -136,19 +136,25 @@ impl Middleware for AuthMiddleware {
// falls back to the host, but we cache the result per host so if a keyring
// implementation returns different credentials for different URLs in the
// same realm we will use the wrong credentials.
- } else if let Some(credentials) = self.keyring.as_ref().and_then(|keyring| {
- if let Some(username) = credentials
- .get()
- .and_then(|credentials| credentials.username())
- {
- debug!("Checking keyring for credentials for {url}");
- // how to fix this
- keyring.fetch(request.url(), username)
- } else {
- trace!("Skipping keyring lookup for {url} with no username");
- None
+ } else if let Some(credentials) = match self.keyring {
+ Some(ref keyring) => {
+ match credentials
+ .get()
+ .and_then(|credentials| credentials.username())
+ {
+ Some(username) => {
+ debug!("Checking keyring for credentials for {url}");
+ // how to fix this
+ keyring.fetch(request.url(), username).await
+ }
+ None => {
+ trace!("Skipping keyring lookup for {url} with no username");
+ None
+ }
+ }
}
- }) {
+ None => None,
+ } {
debug!("Found credentials in keyring for {url}");
request = credentials.authenticate(request);
new_credentials = Some(Arc::new(credentials));
Then you'll run into problems with the Mutex
being synchronous.
Fyi we're chatting about using the tokio mutex over in #3105 – it might not be necessary. |
Ok, i see. Now i split the cache lock to avoid across |
Hey @zanieb, i think this pr is ready to review, let me know if there's anything else i need to do. |
Ah it looks like you have some conflicts with my changes in #3130, sorry about that lmk if you want me to fix it. We also might want to switch to the async mutex since we do want to hold across await points now, interesting. We're also considering switching to a |
OK, thanks for you information, I think it makes sense to split these changes into multiple PR, what do you think? |
Thank you! |
To resolve #3073