wasi-http: Migrate to more descriptive error variant (#7434)

* Migrate to a more specific error-code variant in wasi-http Co-authored-by: Pat Hickey <[email protected]> * Optional fields, and align with upstream pr * Update for upstream changes to the error-code variant * Sync with the upstream implementation * Missed updating an error for riscv64 and s390x * More debuggable error prtest:full * Try to stabilize the test on windows --------- Co-authored-by: Pat Hickey <[email protected]>
bytecodealliance · Nov 11, 2023 · 8b523e7 · 8b523e7
1 parent 6e26b10
commit 8b523e7
Show file tree

Hide file tree

Showing 17 changed files with 359 additions and 248 deletions.
diff --git a/crates/test-programs/src/bin/api_proxy_streaming.rs b/crates/test-programs/src/bin/api_proxy_streaming.rs
@@ -451,7 +451,7 @@ mod executor {
 
     pub fn outgoing_request_send(
         request: OutgoingRequest,
-    ) -> impl Future<Output = Result<IncomingResponse, types::Error>> {
+    ) -> impl Future<Output = Result<IncomingResponse, types::ErrorCode>> {
         future::poll_fn({
             let response = outgoing_handler::handle(request, None);
 

diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_dnsname.rs
@@ -1,4 +1,4 @@
-use test_programs::wasi::http::types::{Method, Scheme};
+use test_programs::wasi::http::types::{ErrorCode, Method, Scheme};
 
 fn main() {
     let res = test_programs::http::request(
@@ -10,9 +10,13 @@ fn main() {
         None,
     );
 
-    let error = res.unwrap_err().to_string();
+    let e = res.unwrap_err();
     assert!(
-        error.starts_with("Error::InvalidUrl(\""),
-        "bad error: {error}"
+        matches!(
+            e.downcast_ref::<ErrorCode>()
+                .expect("expected a wasi-http ErrorCode"),
+            ErrorCode::DnsError(_) | ErrorCode::ConnectionRefused,
+        ),
+        "Unexpected error: {e:#?}"
     );
 }
diff --git a/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs b/crates/test-programs/src/bin/http_outbound_request_invalid_version.rs
@@ -1,17 +1,14 @@
-use test_programs::wasi::http::types::{Method, Scheme};
+use test_programs::wasi::http::types::{ErrorCode, Method, Scheme};
 
 fn main() {
     let addr = std::env::var("HTTP_SERVER").unwrap();
     let res =
         test_programs::http::request(Method::Connect, Scheme::Http, &addr, "/", None, Some(&[]));
 
-    let error = res.unwrap_err().to_string();
-    if !error.starts_with("Error::ProtocolError(\"") {
-        panic!(
-            r#"assertion failed: `(left == right)`
-      left: `"{error}"`,
-      right: `"Error::ProtocolError(\"invalid HTTP version parsed\")"`
-            or `"Error::ProtocolError(\"operation was canceled\")"`)"#
-        )
-    }
+    assert!(matches!(
+        res.unwrap_err()
+            .downcast::<ErrorCode>()
+            .expect("expected a wasi-http ErrorCode"),
+        ErrorCode::HttpProtocolError,
+    ));
 }
diff --git a/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs b/crates/test-programs/src/bin/http_outbound_request_unknown_method.rs
@@ -2,17 +2,14 @@ use test_programs::wasi::http::types::{Method, Scheme};
 
 fn main() {
     let res = test_programs::http::request(
-        Method::Other("OTHER".to_owned()),
+        Method::Other("bad\nmethod".to_owned()),
         Scheme::Http,
         "localhost:3000",
         "/",
         None,
         None,
     );
 
-    let error = res.unwrap_err();
-    assert_eq!(
-        error.to_string(),
-        "Error::InvalidUrl(\"unknown method OTHER\")"
-    );
+    // This error arises from input validation in the `set_method` function on `OutgoingRequest`.
+    assert_eq!(res.unwrap_err().to_string(), "failed to set method");
 }
diff --git a/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs b/crates/test-programs/src/bin/http_outbound_request_unsupported_scheme.rs
@@ -1,4 +1,4 @@
-use test_programs::wasi::http::types::{Method, Scheme};
+use test_programs::wasi::http::types::{ErrorCode, Method, Scheme};
 
 fn main() {
     let res = test_programs::http::request(
@@ -10,9 +10,10 @@ fn main() {
         None,
     );
 
-    let error = res.unwrap_err();
-    assert_eq!(
-        error.to_string(),
-        "Error::InvalidUrl(\"unsupported scheme WS\")"
-    );
+    assert!(matches!(
+        res.unwrap_err()
+            .downcast::<ErrorCode>()
+            .expect("expected a wasi-http ErrorCode"),
+        ErrorCode::HttpProtocolError,
+    ));
 }
diff --git a/crates/test-programs/src/http.rs b/crates/test-programs/src/http.rs
@@ -113,19 +113,16 @@ pub fn request(
     http_types::OutgoingBody::finish(outgoing_body, None);
 
     let incoming_response = match future_response.get() {
-        Some(result) => result.map_err(|_| anyhow!("incoming response errored"))?,
+        Some(result) => result.map_err(|()| anyhow!("response already taken"))?,
         None => {
             let pollable = future_response.subscribe();
             pollable.block();
             future_response
                 .get()
                 .expect("incoming response available")
-                .map_err(|_| anyhow!("incoming response errored"))?
+                .map_err(|()| anyhow!("response already taken"))?
         }
-    }
-    // TODO: maybe anything that appears in the Result<_, E> position should impl
-    // Error? anyway, just use its Debug here:
-    .map_err(|e| anyhow!("{e:?}"))?;
+    }?;
 
     drop(future_response);
 

diff --git a/crates/wasi-http/src/body.rs b/crates/wasi-http/src/body.rs
@@ -14,7 +14,7 @@ use wasmtime_wasi::preview2::{
     Subscribe,
 };
 
-pub type HyperIncomingBody = BoxBody<Bytes, types::Error>;
+pub type HyperIncomingBody = BoxBody<Bytes, types::ErrorCode>;
 
 /// Small wrapper around `BoxBody` which adds a timeout to every frame.
 struct BodyWithTimeout {
@@ -45,12 +45,12 @@ impl BodyWithTimeout {
 
 impl Body for BodyWithTimeout {
     type Data = Bytes;
-    type Error = types::Error;
+    type Error = types::ErrorCode;
 
     fn poll_frame(
         self: Pin<&mut Self>,
         cx: &mut Context<'_>,
-    ) -> Poll<Option<Result<Frame<Bytes>, types::Error>>> {
+    ) -> Poll<Option<Result<Frame<Bytes>, types::ErrorCode>>> {
         let me = Pin::into_inner(self);
 
         // If the timeout timer needs to be reset, do that now relative to the
@@ -66,9 +66,7 @@ impl Body for BodyWithTimeout {
         // Register interest in this context on the sleep timer, and if the
         // sleep elapsed that means that we've timed out.
         if let Poll::Ready(()) = me.timeout.as_mut().poll(cx) {
-            return Poll::Ready(Some(Err(types::Error::TimeoutError(
-                "frame timed out".to_string(),
-            ))));
+            return Poll::Ready(Some(Err(types::ErrorCode::ConnectionReadTimeout)));
         }
 
         // Without timeout business now handled check for the frame. If a frame
@@ -222,7 +220,7 @@ impl Subscribe for HostIncomingBodyStream {
 }
 
 impl HostIncomingBodyStream {
-    fn record_frame(&mut self, frame: Option<Result<Frame<Bytes>, types::Error>>) {
+    fn record_frame(&mut self, frame: Option<Result<Frame<Bytes>, types::ErrorCode>>) {
         match frame {
             Some(Ok(frame)) => match frame.into_data() {
                 // A data frame was received, so queue up the buffered data for
@@ -300,7 +298,7 @@ pub enum HostFutureTrailers {
     ///
     /// Note that `Ok(None)` means that there were no trailers for this request
     /// while `Ok(Some(_))` means that trailers were found in the request.
-    Done(Result<Option<FieldMap>, types::Error>),
+    Done(Result<Option<FieldMap>, types::ErrorCode>),
 }
 
 #[async_trait::async_trait]
@@ -328,9 +326,7 @@ impl Subscribe for HostFutureTrailers {
                 // this just in case though.
                 Err(_) => {
                     debug_assert!(false, "should be unreachable");
-                    *self = HostFutureTrailers::Done(Err(types::Error::ProtocolError(
-                        "stream hung up before trailers were received".to_string(),
-                    )));
+                    *self = HostFutureTrailers::Done(Err(types::ErrorCode::ConnectionTerminated));
                 }
             }
         }
@@ -362,7 +358,7 @@ impl Subscribe for HostFutureTrailers {
     }
 }
 
-pub type HyperOutgoingBody = BoxBody<Bytes, types::Error>;
+pub type HyperOutgoingBody = BoxBody<Bytes, types::ErrorCode>;
 
 pub enum FinishMessage {
     Finished,
@@ -384,7 +380,7 @@ impl HostOutgoingBody {
         }
         impl Body for BodyImpl {
             type Data = Bytes;
-            type Error = types::Error;
+            type Error = types::ErrorCode;
             fn poll_frame(
                 mut self: Pin<&mut Self>,
                 cx: &mut Context<'_>,
@@ -406,9 +402,9 @@ impl HostOutgoingBody {
                                     FinishMessage::Trailers(trailers) => {
                                         Poll::Ready(Some(Ok(Frame::trailers(trailers))))
                                     }
-                                    FinishMessage::Abort => Poll::Ready(Some(Err(
-                                        types::Error::ProtocolError("response corrupted".into()),
-                                    ))),
+                                    FinishMessage::Abort => {
+                                        Poll::Ready(Some(Err(types::ErrorCode::HttpProtocolError)))
+                                    }
                                 },
                                 Poll::Ready(Err(RecvError { .. })) => Poll::Ready(None),
                             }

diff --git a/crates/wasi-http/src/http_impl.rs b/crates/wasi-http/src/http_impl.rs
@@ -1,22 +1,22 @@
-use crate::bindings::http::{
-    outgoing_handler,
-    types::{self as http_types, Scheme},
+use crate::{
+    bindings::http::{
+        outgoing_handler,
+        types::{self, Scheme},
+    },
+    types::{HostFutureIncomingResponse, HostOutgoingRequest, OutgoingRequest},
+    WasiHttpView,
 };
-use crate::types::{self, HostFutureIncomingResponse, OutgoingRequest};
-use crate::WasiHttpView;
 use bytes::Bytes;
 use http_body_util::{BodyExt, Empty};
 use hyper::Method;
-use types::HostOutgoingRequest;
 use wasmtime::component::Resource;
 
 impl<T: WasiHttpView> outgoing_handler::Host for T {
     fn handle(
         &mut self,
         request_id: Resource<HostOutgoingRequest>,
-        options: Option<Resource<http_types::RequestOptions>>,
-    ) -> wasmtime::Result<Result<Resource<HostFutureIncomingResponse>, outgoing_handler::Error>>
-    {
+        options: Option<Resource<types::RequestOptions>>,
+    ) -> wasmtime::Result<Result<Resource<HostFutureIncomingResponse>, types::ErrorCode>> {
         let opts = options.and_then(|opts| self.table().get(&opts).ok());
 
         let connect_timeout = opts
@@ -32,32 +32,30 @@ impl<T: WasiHttpView> outgoing_handler::Host for T {
             .unwrap_or(std::time::Duration::from_millis(600 * 1000));
 
         let req = self.table().delete(request_id)?;
+        let mut builder = hyper::Request::builder();
 
-        let method = match req.method {
-            crate::bindings::http::types::Method::Get => Method::GET,
-            crate::bindings::http::types::Method::Head => Method::HEAD,
-            crate::bindings::http::types::Method::Post => Method::POST,
-            crate::bindings::http::types::Method::Put => Method::PUT,
-            crate::bindings::http::types::Method::Delete => Method::DELETE,
-            crate::bindings::http::types::Method::Connect => Method::CONNECT,
-            crate::bindings::http::types::Method::Options => Method::OPTIONS,
-            crate::bindings::http::types::Method::Trace => Method::TRACE,
-            crate::bindings::http::types::Method::Patch => Method::PATCH,
-            crate::bindings::http::types::Method::Other(method) => {
-                return Ok(Err(outgoing_handler::Error::InvalidUrl(format!(
-                    "unknown method {method}"
-                ))));
-            }
-        };
+        builder = builder.method(match req.method {
+            types::Method::Get => Method::GET,
+            types::Method::Head => Method::HEAD,
+            types::Method::Post => Method::POST,
+            types::Method::Put => Method::PUT,
+            types::Method::Delete => Method::DELETE,
+            types::Method::Connect => Method::CONNECT,
+            types::Method::Options => Method::OPTIONS,
+            types::Method::Trace => Method::TRACE,
+            types::Method::Patch => Method::PATCH,
+            types::Method::Other(m) => match hyper::Method::from_bytes(m.as_bytes()) {
+                Ok(method) => method,
+                Err(_) => return Ok(Err(types::ErrorCode::HttpRequestMethodInvalid)),
+            },
+        });
 
         let (use_tls, scheme, port) = match req.scheme.unwrap_or(Scheme::Https) {
-            Scheme::Http => (false, "http://", 80),
-            Scheme::Https => (true, "https://", 443),
-            Scheme::Other(scheme) => {
-                return Ok(Err(outgoing_handler::Error::InvalidUrl(format!(
-                    "unsupported scheme {scheme}"
-                ))))
-            }
+            Scheme::Http => (false, http::uri::Scheme::HTTP, 80),
+            Scheme::Https => (true, http::uri::Scheme::HTTPS, 443),
+
+            // We can only support http/https
+            Scheme::Other(_) => return Ok(Err(types::ErrorCode::HttpProtocolError)),
         };
 
         let authority = if let Some(authority) = req.authority {
@@ -69,24 +67,32 @@ impl<T: WasiHttpView> outgoing_handler::Host for T {
         } else {
             String::new()
         };
+        builder = builder.header(hyper::header::HOST, &authority);
+
+        let mut uri = http::Uri::builder()
+            .scheme(scheme)
+            .authority(authority.clone());
+
+        if let Some(path) = req.path_with_query {
+            uri = uri.path_and_query(path);
+        }
 
-        let mut builder = hyper::Request::builder()
-            .method(method)
-            .uri(format!(
-                "{scheme}{authority}{}",
-                req.path_with_query.as_ref().map_or("", String::as_ref)
-            ))
-            .header(hyper::header::HOST, &authority);
+        builder = builder.uri(uri.build().map_err(|e| {
+            eprintln!("uri build error: {e:#?}");
+            types::ErrorCode::HttpRequestUriInvalid
+        })?);
 
         for (k, v) in req.headers.iter() {
             builder = builder.header(k, v);
         }
 
         let body = req
             .body
-            .unwrap_or_else(|| Empty::<Bytes>::new().map_err(|_| unreachable!()).boxed());
+            .unwrap_or_else(|| Empty::<Bytes>::new().map_err(|_| todo!("thing")).boxed());
 
-        let request = builder.body(body).map_err(types::http_protocol_error)?;
+        let request = builder
+            .body(body)
+            .map_err(|err| types::ErrorCode::InternalError(Some(err.to_string())))?;
 
         Ok(Ok(self.send_request(OutgoingRequest {
             use_tls,