Optimize Fetcher::find and fix quickinstall reporting (#579)

* Optimize `Fetcher::find`: Make it non-async to avoid boxing
   and return `AutoAbortJoinHandle<...>` instead.
   
   Since spawning a new task in tokio box the future anyway, boxing the
   returned future again in `Fetcher::find` merely adds unnecessary
   overheads.
* Optimize `QuickInstall::report`: Make it async fn instead of ret `JoinHandle`
   
   This provides several benefits:
    - On debug build, no task will be spawned
    - The calls to `self.stats_url()` can be evaluated in parallel.
    - The task now returns `()` so that the task spawned is smaller.
* Fix `QuickInstall::find`: `warn!` if quickinstall report fails

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

crates/binstalk/src/fetchers.rs

@@ -6,7 +6,7 @@ pub use quickinstall::*;
 
 use crate::{
     errors::BinstallError,
-    helpers::remote::Client,
+    helpers::{remote::Client, tasks::AutoAbortJoinHandle},
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
 
@@ -32,7 +32,7 @@ pub trait Fetcher: Send + Sync {
     ///
     /// Must return `true` if a package is available, `false` if none is, and reserve errors to
     /// fatal conditions only.
-    async fn find(&self) -> Result<bool, BinstallError>;
+    fn find(self: Arc<Self>) -> AutoAbortJoinHandle<Result<bool, BinstallError>>;
 
     /// Return the package format
     fn pkg_fmt(&self) -> PkgFmt;

crates/binstalk/src/fetchers/gh_crate_meta.rs

@@ -16,6 +16,7 @@ use crate::{
         download::Download,
         remote::{Client, Method},
         signal::wait_on_cancellation_signal,
+        tasks::AutoAbortJoinHandle,
     },
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
@@ -83,26 +84,27 @@ impl super::Fetcher for GhCrateMeta {
         })
     }
 
-    async fn find(&self) -> Result<bool, BinstallError> {
-        let repo = if let Some(repo) = self.data.repo.as_deref() {
-            Some(
-                self.client
-                    .get_redirected_final_url(Url::parse(repo)?)
-                    .await?,
-            )
-        } else {
-            None
-        };
-
-        let pkg_urls = if let Some(pkg_url) = self.target_data.meta.pkg_url.as_deref() {
-            Either::Left(pkg_url)
-        } else if let Some(repo) = repo.as_ref() {
-            if let Some(pkg_urls) =
-                RepositoryHost::guess_git_hosting_services(repo)?.get_default_pkg_url_template()
-            {
-                Either::Right(pkg_urls)
+    fn find(self: Arc<Self>) -> AutoAbortJoinHandle<Result<bool, BinstallError>> {
+        AutoAbortJoinHandle::spawn(async move {
+            let repo = if let Some(repo) = self.data.repo.as_deref() {
+                Some(
+                    self.client
+                        .get_redirected_final_url(Url::parse(repo)?)
+                        .await?,
+                )
             } else {
-                warn!(
+                None
+            };
+
+            let pkg_urls = if let Some(pkg_url) = self.target_data.meta.pkg_url.as_deref() {
+                Either::Left(pkg_url)
+            } else if let Some(repo) = repo.as_ref() {
+                if let Some(pkg_urls) =
+                    RepositoryHost::guess_git_hosting_services(repo)?.get_default_pkg_url_template()
+                {
+                    Either::Right(pkg_urls)
+                } else {
+                    warn!(
                     concat!(
                         "Unknown repository {}, cargo-binstall cannot provide default pkg_url for it.\n",
                         "Please ask the upstream to provide it for target {}."
@@ -110,10 +112,10 @@ impl super::Fetcher for GhCrateMeta {
                     repo, self.target_data.target
                 );
 
-                return Ok(false);
-            }
-        } else {
-            warn!(
+                    return Ok(false);
+                }
+            } else {
+                warn!(
                 concat!(
                     "Package does not specify repository, cargo-binstall cannot provide default pkg_url for it.\n",
                     "Please ask the upstream to provide it for target {}."
@@ -121,39 +123,44 @@ impl super::Fetcher for GhCrateMeta {
                 self.target_data.target
             );
 
-            return Ok(false);
-        };
+                return Ok(false);
+            };
 
-        // Convert Option<Url> to Option<String> to reduce size of future.
-        let repo = repo.map(String::from);
-        let repo = repo.as_deref().map(|u| u.trim_end_matches('/'));
+            // Convert Option<Url> to Option<String> to reduce size of future.
+            let repo = repo.map(String::from);
+            let repo = repo.as_deref().map(|u| u.trim_end_matches('/'));
 
-        let launch_baseline_find_tasks = |pkg_fmt| {
-            match &pkg_urls {
-                Either::Left(pkg_url) => Either::Left(iter::once(*pkg_url)),
-                Either::Right(pkg_urls) => Either::Right(pkg_urls.iter().map(Deref::deref)),
+            // Use reference to self to fix error of closure
+            // launch_baseline_find_tasks which moves `this`
+            let this = &self;
+
+            let launch_baseline_find_tasks = |pkg_fmt| {
+                match &pkg_urls {
+                    Either::Left(pkg_url) => Either::Left(iter::once(*pkg_url)),
+                    Either::Right(pkg_urls) => Either::Right(pkg_urls.iter().map(Deref::deref)),
+                }
+                .flat_map(move |pkg_url| this.launch_baseline_find_tasks(pkg_fmt, pkg_url, repo))
+            };
+
+            let mut handles: FuturesUnordered<_> =
+                if let Some(pkg_fmt) = self.target_data.meta.pkg_fmt {
+                    launch_baseline_find_tasks(pkg_fmt).collect()
+                } else {
+                    PkgFmt::iter()
+                        .flat_map(launch_baseline_find_tasks)
+                        .collect()
+                };
+
+            while let Some(res) = handles.next().await {
+                if let Some((url, pkg_fmt)) = res? {
+                    debug!("Winning URL is {url}, with pkg_fmt {pkg_fmt}");
+                    self.resolution.set((url, pkg_fmt)).unwrap(); // find() is called first
+                    return Ok(true);
+                }
             }
-            .flat_map(move |pkg_url| self.launch_baseline_find_tasks(pkg_fmt, pkg_url, repo))
-        };
 
-        let mut handles: FuturesUnordered<_> = if let Some(pkg_fmt) = self.target_data.meta.pkg_fmt
-        {
-            launch_baseline_find_tasks(pkg_fmt).collect()
-        } else {
-            PkgFmt::iter()
-                .flat_map(launch_baseline_find_tasks)
-                .collect()
-        };
-
-        while let Some(res) = handles.next().await {
-            if let Some((url, pkg_fmt)) = res? {
-                debug!("Winning URL is {url}, with pkg_fmt {pkg_fmt}");
-                self.resolution.set((url, pkg_fmt)).unwrap(); // find() is called first
-                return Ok(true);
-            }
-        }
-
-        Ok(false)
+            Ok(false)
+        })
     }
 
     async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> {

crates/binstalk/src/fetchers/quickinstall.rs

@@ -1,8 +1,7 @@
 use std::{path::Path, sync::Arc};
 
 use compact_str::CompactString;
-use tokio::task::JoinHandle;
-use tracing::debug;
+use tracing::{debug, warn};
 use url::Url;
 
 use crate::{
@@ -11,6 +10,7 @@ use crate::{
         download::Download,
         remote::{Client, Method},
         signal::wait_on_cancellation_signal,
+        tasks::AutoAbortJoinHandle,
     },
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
@@ -43,14 +43,29 @@ impl super::Fetcher for QuickInstall {
         })
     }
 
-    async fn find(&self) -> Result<bool, BinstallError> {
-        let url = self.package_url();
-        self.report();
-        debug!("Checking for package at: '{url}'");
-        Ok(self
-            .client
-            .remote_exists(Url::parse(&url)?, Method::HEAD)
-            .await?)
+    fn find(self: Arc<Self>) -> AutoAbortJoinHandle<Result<bool, BinstallError>> {
+        AutoAbortJoinHandle::spawn(async move {
+            if cfg!(debug_assertions) {
+                debug!("Not sending quickinstall report in debug mode");
+            } else {
+                let this = self.clone();
+                tokio::spawn(async move {
+                    if let Err(err) = this.report().await {
+                        warn!(
+                            "Failed to send quickinstall report for package {}: {err}",
+                            this.package
+                        )
+                    }
+                });
+            }
+
+            let url = self.package_url();
+            debug!("Checking for package at: '{url}'");
+            Ok(self
+                .client
+                .remote_exists(Url::parse(&url)?, Method::HEAD)
+                .await?)
+        })
     }
 
     async fn fetch_and_extract(&self, dst: &Path) -> Result<(), BinstallError> {
@@ -110,22 +125,12 @@ impl QuickInstall {
         )
     }
 
-    pub fn report(&self) -> JoinHandle<Result<(), BinstallError>> {
-        let stats_url = self.stats_url();
-        let client = self.client.clone();
+    pub async fn report(&self) -> Result<(), BinstallError> {
+        let url = Url::parse(&self.stats_url())?;
+        debug!("Sending installation report to quickinstall ({url})");
 
-        tokio::spawn(async move {
-            if cfg!(debug_assertions) {
-                debug!("Not sending quickinstall report in debug mode");
-                return Ok(());
-            }
+        self.client.remote_exists(url, Method::HEAD).await?;
 
-            let url = Url::parse(&stats_url)?;
-            debug!("Sending installation report to quickinstall ({url})");
-
-            client.remote_exists(url, Method::HEAD).await?;
-
-            Ok(())
-        })
+        Ok(())
     }
 }

crates/binstalk/src/ops/resolve.rs

@@ -20,7 +20,7 @@ use crate::{
     drivers::fetch_crate_cratesio,
     errors::BinstallError,
     fetchers::{Data, Fetcher, TargetData},
-    helpers::{remote::Client, tasks::AutoAbortJoinHandle},
+    helpers::remote::Client,
     manifests::cargo_toml_binstall::{Meta, PkgMeta, PkgOverride},
 };
 
@@ -162,10 +162,7 @@ async fn resolve_inner(
             .cartesian_product(resolvers)
             .map(|(target_data, f)| {
                 let fetcher = f(opts.client.clone(), data.clone(), target_data);
-                (
-                    fetcher.clone(),
-                    AutoAbortJoinHandle::spawn(async move { fetcher.find().await }),
-                )
+                (fetcher.clone(), fetcher.find())
             }),
     );