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

feat(ext/fetch): Replace reqwest with reqwest_middleware #23539

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

maxmcd
Copy link

@maxmcd maxmcd commented Apr 24, 2024

Related to: #23516

This replaces uses of reqwest and reqwest::Client with the request_middleware alternatives so that middleware can be mounted. The intention is to allow tracing and other middleware to be added in Rust code.

I considered adding the middleware configuration to CreateHttpClientOptions, but it was a little tricky to plumb it through everywhere. Hopefully this use of once_cell is considered ok and fits well with the contract of this middleware, ie: "add middleware to all deno http requests".

Example of use:

diff --git a/cli/main.rs b/cli/main.rs
index a4e93ca31..61a969bb5 100644
--- a/cli/main.rs
+++ b/cli/main.rs
@@ -41,6 +41,9 @@ use deno_core::error::JsError;
 use deno_core::futures::FutureExt;
 use deno_core::unsync::JoinHandle;
 use deno_npm::resolution::SnapshotFromLockfileError;
+use deno_runtime::deno_fetch::add_middleware;
+use deno_runtime::deno_fetch::reqwest;
+use deno_runtime::deno_fetch::reqwest_middleware;
 use deno_runtime::fmt_errors::format_js_error;
 use deno_runtime::tokio_util::create_and_run_current_thread_with_maybe_metrics;
 use deno_terminal::colors;
@@ -50,6 +53,7 @@ use std::env;
 use std::env::current_exe;
 use std::future::Future;
 use std::path::PathBuf;
+use std::sync::Arc;

 /// Ensures that all subcommands return an i32 exit code and an [`AnyError`] error type.
 trait SubcommandOutput {
@@ -302,10 +306,28 @@ pub(crate) fn unstable_warn_cb(feature: &str, api_name: &str) {
     ))
   );
 }
+struct LoggingMiddleware;
+
+#[async_trait::async_trait]
+impl reqwest_middleware::Middleware for LoggingMiddleware {
+  async fn handle(
+    &self,
+    req: reqwest::Request,
+    extensions: &mut task_local_extensions::Extensions,
+    next: reqwest_middleware::Next<'_>,
+  ) -> reqwest_middleware::Result<reqwest::Response> {
+    println!("Request started {:?}", req);
+    let res = next.run(req, extensions).await;
+    println!("Result: {:?}", res);
+    res
+  }
+}

 pub fn main() {
   setup_panic_hook();

+  add_middleware(Arc::new(LoggingMiddleware)).unwrap();
+
   util::unix::raise_fd_limit();
   util::windows::ensure_stdio_open();
   #[cfg(windows)]

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2024

CLA assistant check
All committers have signed the CLA.

@maxmcd maxmcd marked this pull request as ready for review April 24, 2024 17:21
) -> Result<(), AnyError> {
HTTP_MIDDLEWARE
.lock()
.map_err(|_e| generic_error("cannot acquire poisoned lock"))?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to handle this error? I tried e.into() but could not figure out the compiler error.

@@ -126,7 +126,7 @@ impl<P: RemoteDbHandlerPermissions + 'static> DatabaseHandler
};

let options = &self.http_options;
let client = create_http_client(
let client = create_http_client_without_middleware(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing kv middleware. I keep reqwest::Client here since denokv_remote wants one.

@maxmcd
Copy link
Author

maxmcd commented Apr 24, 2024

Ah, the testing is tricky because I am using a global variable and the tokio tests are run async so the middleware is picked up by something else. Maybe there is a way to lock a specific test so I can ensure I have cleaned up?

@bartlomieju bartlomieju added this to the 1.44 milestone May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants