-
Notifications
You must be signed in to change notification settings - Fork 645
Add procedure HTTP request API for WASM modules and the Rust module bindings library #3684
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
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.
Despite it appearing locally
|
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 |
Error-handling changes to follow
This appears to be our only use of `NodesError::DecodeValue`, so I just made `err_to_errno` treat that variant as `BSATN_DECODE_ERROR`.
# Description of Changes Follow up to #3684. Moves `Error` and `Timeout` out of lib, so that we don't have to implement `SpacetimeType` for them, and then removes the http dependency altogether, so that `lib` can be leaner. I also got rid of the separate `HttpValue` type, since it only really exists to mirror the `http` crate and typescript won't make use of it. # Expected complexity level and risk 1 # Testing n/a - just code movement.
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 .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). 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
sdk-testsuite,procedure::http_okandprocedure::http_err, which make successful and failing requests respectively, then return its result. A client then makes some assertions about the result.