-
Notifications
You must be signed in to change notification settings - Fork 666
C# bindings: add procedure_http_request support + fix HTTP BSATN wire format to match spacetimedb_lib::http
#3944
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
Signed-off-by: Ryan <[email protected]>
gefjon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like review from @JasonAtClockwork , as he's more familiar with C# than I am, but this looks good to me. I think we should be at feature parity with Rust and TypeScript enough to add a module sdk-test-procedure-cs and to use it in the procedure_tests in sdks/rust/tests/test.rs. I'll create a ticket for that, and we can discuss how to prioritize it.
JasonAtClockwork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I ran the tests with diving in a bit and added extra debugging to see the output which also looks great.
|
One comment I wanted to keep separate from the review, I think this officially means we can kill the /sdks/csharp/examples~/regression-tests/procedure-client. Should we make a separate issue for that? It's not an emergency but will speed up regression tests a bit. |
Great point! Created #3954 so we can track this. |
Description of Changes
Rust added procedure-scoped HTTP via the
procedure_http_requestABI (see Rust PR #3684) to C# host bindings.What changed:
BSATN.Runtime/HttpWireTypes.cs) to match exactly the Rust stable types:spacetimedb_lib::http::Requestfields and order:method,headers,timeout,uri,versionspacetimedb_lib::http::Responsefields and order:headers,version,codename: string, value: bytes); no additional metadata is encoded.ProcedureContext.Httpsupport (Runtime/Http.cs) that:HttpRequestWire+ sends body bytes viaprocedure_http_requestHttpResponseWireand returns(response, body)as a typedHttpResponseHTTP_ERROR, decodes the error payload as a BSATNstringWOULD_BLOCK_TRANSACTION, returns a stable error message (host rejects blocking HTTP while a mut tx is open)procedure_http_requestimport to the C# wasm shim (Runtime/bindings.c) under thespacetime_10.3import module.[BytesSource; 2]).Runtime/Internal/Module.call_procedure): it must either returnErrno.OKor trap (log + rethrow). This mirrors the host’s expectations and avoids “unexpected errno values from guest entrypoints” behavior.Behavioral notes vs Rust
expect(...)/panic on “should never happen” serialization failures; C# runtime explicitly avoids throwing across the procedure boundary for the HTTP client path and instead returnsResult.Errfor unexpected failures. This prevents whole-module traps from user-space HTTP usage while still surfacing rich errors.API and ABI breaking changes
No new host ABI introduced, and does not change the syscall signature.
Adds API:
ProcedureContextBase.Httpis available for proceduresNo existing public types/method signatures are removed
expect(...)s and may panic/trap on internal “should not happen” cases. On the other hand, C#'sHttpClient.Sendexplicitly converts unexpected failures intoResult.Errto avoid trapping the module. This is a behavioral difference, but not an API break.Expected complexity level and risk
2 - Mostly just creating equivalents to the Rust version.
Testing
C# regression tests updated to cover:
ReadMySchemaViaHttp)InvalidHttpRequest) returning error without trappingTesting performed:
Success.