Conversation
Each process_response call takes the OHTTP context out of the ClientResponse it is given. Bindings hand ClientResponse out as a shared object and pass it by reference, so nothing stops a caller from passing the same one twice, for example when retrying after a transient failure. The second call panicked, which crosses the FFI boundary as an opaque exception, or aborts the process when built with panic=abort. Make the conversion fallible and return a new ClientResponseError from every process_response and process_error_response instead. These methods now throw in the foreign bindings; their return types are unchanged.
Collaborator
Coverage Report for CI Build 35959851359Coverage remained the same at 86.869%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
ClientResponsecan only be used once, becauseprocess_responsetakes the OHTTP context out of it. Nothing in the bindings says so: the object is handed out as a shared reference and everyprocess_responseborrows it. A caller that retries after a transient failure with the sameClientResponsehitexpect("ClientResponse moved out of memory"). Across uniffi that panic reaches the caller as an opaque internal error, or aborts the process in builds withpanic = "abort".This makes the conversion fallible and adds
ClientResponseError::AlreadyUsed. The five methods that take aClientResponsereturn it instead of panicking:WithReplyKey::process_responsePollingForProposal::process_responseInitialized::process_responsePayjoinProposal::process_responseHasReplyableError::process_error_responseIn Rust these now return
Result<Transition, ClientResponseError>. In the generated bindings the return types are unchanged and the methods can now throw the new error (ClientResponseExceptionin Kotlin,ClientResponseError.AlreadyUsedin Python). Existing binding code compiles as is. To retry, create a new request, which gives a fresh context. Thepayjoincrate is untouched.The new sender and receiver tests fail a first
process_responsetransiently with an undersized body, then call it again with the same context and expectAlreadyUsed. Both panicked on master.The transition
save()methods have the same take-then-expect pattern. I left them out to keep this change small.Disclosure: co-authored by Claude Code. The bug was found by an automated code scanner.