Add test: FetchOhttpKeys in non-Tokio context#1351
Add test: FetchOhttpKeys in non-Tokio context#1351ValeraFinebits wants to merge 3 commits intopayjoin:masterfrom
Conversation
Pull Request Test Coverage Report for Build 22277851434Details
💛 - Coveralls |
FetchOhttpKeys is exported as a UniFFI async function and is polled from plain .NET async threads in the C# binding. Those threads do not carry a Tokio runtime, so reqwest panics with "there is no reactor running" when the future is polled. Enable UniFFI's tokio feature and mark fetch_ohttp_keys with #[uniffi::export(async_runtime = "tokio")] so UniFFI polls it inside a Tokio-compatible runtime context. This fixes the runtime panic path and allows errors to be surfaced as regular IO failures instead of a PanicException.
|
I think exposing something like this for convenience is fine but really what you want to do is re-implement the fetch with a native HTTP client. Then use that impl in the integration test and ship it. Kind of silly to compile reqwest in for this one function. This is true for all of the foreign language implementations. |
After enabling Tokio runtime polling for UniFFI async IO, the C# non-Tokio regression moved past PanicException and began failing with an IoException against local HTTPS test services. That failure was a TLS trust issue: TestServices uses a generated self-signed certificate, while FetchOhttpKeys uses default trust. The regression should validate async runtime behavior, not certificate store configuration. Switch the regression to FetchOhttpKeysWithCert and pass TestServices certificate bytes. Also export fetch_ohttp_keys_with_cert with Tokio runtime polling and include _manual-tls in default C# binding generation features so local and CI builds stay consistent.
|
I reproduced the failure and confirmed two separate issues in sequence:
Follow-up fix:
Result locally:
|
|
If this unblocks you it's totally ok to move it forward but even medium term (and with llms making this imo fully define problem easy to do now) I suggest superseding it soon |
@DanGould Thanks for the context. Do you expect fetch_ohttp_keys / fetch_ohttp_keys_with_cert to remain part of the public UniFFI API, or are you planning to deprecate/remove them? |
|
At this point it may be best to remove them from the UniFFI API and ensure they are implemented + tested natively in all downstream languages |
5 participants
PayjoinMethods.FetchOhttpKeys() panics with "there is no reactor running" when called from a plain .NET async context, because UniFFI polls the Rust future on a .NET thread-pool thread that has no Tokio runtime, and reqwest requires one.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.