Add procedure HTTP request API for WASM modules and the Rust module bindings library #3684
Conversation
With this PR, procedures (at least, those defined in Rust modules) can perform HTTP requests! This is performed through a new field on the `ProcedureContext`, `http: HttpClient`, which has a method `send` for sending an `http::Request`, as well as a convenience wrapper `get`. Internally, these methods hit the `procedure_http_request` ABI call / host function, which uses reqwest to perform an HTTP request. The request is run with a user-configurable timeout which defaults and is clamped to 500 ms. Rather than exposing the HTTP stream to modules, we download the entire response body immediately, within the same timeout. I've added an example usage of `get` to `module-test` which performs a request against `localhost:3000` to read its own schema/moduledef. Not included in this commit, but intended to be within this PR, are: - [ ] A test using the `sdk-test` framework. - [ ] Expanded documentation. Left as TODOs are: - Metrics for recording request and response size. - Improving performance by stashing a long-lived `reqwest::Client` someplace. Currently we build a new `Client` for each request. - Improving performance (possibly) by passing the request-future to the global tokio executor rather than running it on the single-threaded database executor.
| #[sats(crate = crate)] | ||
| enum HttpRequest { | ||
| V0(HttpRequestV0), | ||
| NonExhaustive(Box<[u8]>), |
There was a problem hiding this comment.
What's the purpose of this? Why not just #[non_exhaustive]? And I think it'd actually be better to not mark it non_exhaustive, since we want to remember to catch all the places we match on it if we do add a variant, and this isn't really user-facing.
There was a problem hiding this comment.
This is to allow extensions while preserving BFLATN-compatibility: we can add additional variants at the end of a sum type, but they must have <= the size and alignment of all the existing variants. Ensuring that there's one variant whose payload is a var-length type (String, Vec<u8>, &c) means that, in the worst case, we can add new variants whose SATS type is a bytestring, which we assign additional semantics to, e.g. by treating it as the BSATN of some known type.
There was a problem hiding this comment.
There's some discussion of this in the module-level doc comment. If you think it's useful, I could add comments to each variant like this as well.
There was a problem hiding this comment.
Maybe just a "See module docs..." style comment on each variant?
| }), | ||
| } = req | ||
| else { | ||
| unreachable!("`HttpRequest::NonExhausitve` pseudo-variant encountered"); |
There was a problem hiding this comment.
A user could theoretically pass a NonExhaustive to an abi call, causing a crash.
There was a problem hiding this comment.
Good point. I'll make this a trap rather than a panic.
| struct HttpRequestV0 { | ||
| body: Body, | ||
| method: Method, | ||
| headers: Headers, | ||
| timeout: Option<Timeout>, | ||
| /// A valid URI, sourced from an already-validated [`http::Uri`]. | ||
| uri: String, | ||
| version: Version, | ||
| } |
There was a problem hiding this comment.
I think it might be a good idea to store the body separately from the rest of the request, incl. as a separate argument in the abi call and perhaps even just not defining a type for it in this file. In terms of interchange the body is really straightforward to represent (it's just bytes), but if we did want to support streaming in the future you wouldn't be able to use this. A side benefit of defining it as a separate argument in the abi call is that we don't have to copy the buffer that the user provides.
There was a problem hiding this comment.
What would you use as the bound on T in http::Request<T> as an argument to ctx.http.send?
There was a problem hiding this comment.
Something defined in that crate - there could be a body type equivalent to what's here now, but it should be part of the bindings api, not an interchange type.
Despite it appearing locally
| /// suspending execution until the request is complete, | ||
| /// then return its response via a [`BytesSource`] written to `out`. | ||
| /// | ||
| /// `request_ptr[..request_len]` should store a BSATN-serialized `spacetimedb_lib::http::Request` object |
There was a problem hiding this comment.
| /// `request_ptr[..request_len]` should store a BSATN-serialized `spacetimedb_lib::http::Request` object | |
| /// `request_ptr[..request_len]` should store a BSATN-serialized [`spacetimedb_lib::http::Request`] object |
There was a problem hiding this comment.
I believe that this (and others with similar comments within this crate) isn't a valid link - bindings-sys does not have a dependency on lib.
There was a problem hiding this comment.
Oh, that makes sense; please disregard 😊
| /// containing the details of the request to be performed. | ||
| /// | ||
| /// If the request is successful, a [`BytesSource`] is written to `out` | ||
| /// containing a BSATN-encoded `spacetimedb_lib::http::Response` object. |
There was a problem hiding this comment.
| /// containing a BSATN-encoded `spacetimedb_lib::http::Response` object. | |
| /// containing a BSATN-encoded [`spacetimedb_lib::http::Response`] object. |
| /// If the request is successful, a [`BytesSource`] is written to `out` | ||
| /// containing a BSATN-encoded `spacetimedb_lib::http::Response` object. | ||
| /// "Successful" in this context includes any connection which results in any HTTP status code, | ||
| /// regardless of the specified meaning of that code. |
There was a problem hiding this comment.
| /// regardless of the specified meaning of that code. | |
| /// regardless of the specified meaning of that code. | |
| /// This includes error codes such as 404 Not Found | |
| /// and 500 Internal Server Error. |
| /// In this case, `out` is not written. | ||
| /// - `HTTP_ERROR` if an error occurs while executing the HTTP request. | ||
| /// In this case, a [`BytesSource`] is written to `out` | ||
| /// containing a BSATN-encoded `spacetimedb_lib::http::Error` object. |
There was a problem hiding this comment.
| /// containing a BSATN-encoded `spacetimedb_lib::http::Error` object. | |
| /// containing a BSATN-encoded [`spacetimedb_lib::http::Error`] object. |
| /// suspending execution until the request is complete, | ||
| /// then return its response or error. | ||
| /// | ||
| /// `http_request_bsatn` should be a BSATN-serialized `spacetimedb_lib::http::Request`. |
There was a problem hiding this comment.
| /// `http_request_bsatn` should be a BSATN-serialized `spacetimedb_lib::http::Request`. | |
| /// `http_request_bsatn` should be a BSATN-serialized [`spacetimedb_lib::http::Request`]. |
| /// If the request is successful, a [`BytesSource`] is written to `out` | ||
| /// containing a BSATN-encoded `spacetimedb_lib::http::Response` object. | ||
| /// "Successful" in this context includes any connection which results in any HTTP status code, | ||
| /// regardless of the specified meaning of that code. |
There was a problem hiding this comment.
| /// regardless of the specified meaning of that code. | |
| /// regardless of the specified meaning of that code. | |
| /// This includes error codes such as 404 Not Found | |
| /// and 500 Internal Server Error. |
| // BSATN deserialization failure of the request will trap by returning `NodesError::DecodeValue`, | ||
| // which `Self::convert_wasm_result` treats as fatal. |
There was a problem hiding this comment.
I'd prefer for consistency to return the error code BSATN_DECODE_ERROR.
| //! Using an enum allows us to add additional variants while preserving the BSATN encoding passed across the WASM boundary, | ||
| //! and including a variant with a variable-length type | ||
| //! allows us to add other variants with variable-length types while preserving the BFLATN layout stored in table pages. | ||
| //! (It's unlikely that any of these types will end up stored in a table, but better safe than sorry.) |
There was a problem hiding this comment.
| //! (It's unlikely that any of these types will end up stored in a table, but better safe than sorry.) | |
| //! (It's unlikely that any of these types will end up stored in a table, | |
| //! but better safe than sorry. Since we're doing networking IO anyways, | |
| //! the added cost is negligible.) |
|
|
||
| /// Represents an HTTP request which can be made from a procedure running in a SpacetimeDB database. | ||
| /// | ||
| /// Construct instances of this type by converting from [`http::Request`]. |
There was a problem hiding this comment.
| /// Construct instances of this type by converting from [`http::Request`]. | |
| /// Construct instances of this type by converting from [`http::Request`]. | |
| /// Note that all extensions to `http::Request`, | |
| /// save for [`Timeout`], are ignored. |
|
Phoebe and I were discussing Furthermore, if we did want to change HttpRequest in the future, it'd probably just be best to introduce a new abi function altogther, since that's our existing mechanism for versioning. Under the enum scheme, if we added a new variant and someone built a new module and ran it against an old spacetimedb server, it would be a runtime error only when the http request is actually made. Also, no other types in |
Description of Changes
Closes #3517 .
With this PR, procedures (at least, those defined in Rust modules) can perform HTTP requests! This is performed through a new field on the
ProcedureContext,http: HttpClient, which has a methodsendfor sending anhttp::Request, as well as a convenience wrapperget.Internally, these methods hit the
procedure_http_requestABI call / host function, which uses reqwest to perform an HTTP request. The request is run with a user-configurable timeout which defaults and is clamped to 500 ms.Rather than exposing the HTTP stream to modules, we download the entire response body immediately, within the same timeout.
I've added an example usage of
gettomodule-testwhich performs a request againstlocalhost:3000to read its own schema/moduledef.This PR also makes all procedure-related definitions in the Rust module bindings library
#[cfg(feature = "unstable")], as per #3644 . The rename of the/v1/database/:name/procedure/:nameroute is not included in this PR, so this does not close #3644 .Not included in this commit, but intended to be within this PR, are:
sdk-testframework.Left as TODOs are:
reqwest::Clientsomeplace.Currently we build a new
Clientfor each request.rather than running it on the single-threaded database executor.
API and ABI breaking changes
Adds new APIs, which are marked as unstable. Adds a new ABI, which is not unstable in any meaningful way (we can't really do that), but is carefully made extensible through SATS/BSATN trickery - see module doc comment in
crates/spacetimedb-lib/src/http.rs. Marks unreleased APIs as unstable. Does not affect any pre-existing already-released APIs or ABIs.Expected complexity level and risk
3 or so: networking is scary, and even though we impose a timeout which prevents these connections from being truly long-lived, they're still potentially long-lived on the scale of Tokio futures. It's possible that running them on the database core is problematic in some way, and so what I've left as a performance TODO could actually be a concurrency-correctness issue.
Testing
TODO