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

Make KeyringProvider::fetch_* async #3089

Merged
merged 8 commits into from
Apr 23, 2024
Merged

Make KeyringProvider::fetch_* async #3089

merged 8 commits into from
Apr 23, 2024

Conversation

naosense
Copy link
Contributor

@naosense naosense commented Apr 17, 2024

To resolve #3073

@@ -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)
Copy link
Contributor Author

@naosense naosense Apr 17, 2024

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.

Copy link
Member

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.

@zanieb
Copy link
Member

zanieb commented Apr 18, 2024

Fyi we're chatting about using the tokio mutex over in #3105 – it might not be necessary.

@naosense
Copy link
Contributor Author

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 await call.

@naosense naosense marked this pull request as ready for review April 18, 2024 08:30
@naosense naosense changed the title WIP: Make KeyringProvider::fetch_* async Make KeyringProvider::fetch_* async Apr 18, 2024
@naosense
Copy link
Contributor Author

Hey @zanieb, i think this pr is ready to review, let me know if there's anything else i need to do.

@zanieb zanieb added the performance Potential performance improvement label Apr 22, 2024
@zanieb
Copy link
Member

zanieb commented Apr 23, 2024

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 RwLock. I think all that is probably worth doing/considering separately.

@naosense
Copy link
Contributor Author

OK, thanks for you information, I think it makes sense to split these changes into multiple PR, what do you think?

@zanieb zanieb merged commit 65efaf7 into astral-sh:main Apr 23, 2024
39 checks passed
@zanieb
Copy link
Member

zanieb commented Apr 23, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyringProvider::fetch_subprocess should be async
2 participants