Skip to content

Add support for nested gRPC callouts. #240

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

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

andytesti
Copy link
Contributor

Panic with message "proxy-wasm-rust-sdk/src/dispatcher.rs:121:14:\nalready borrowed: BorrowMutError" is triggered when dispatch_grpc_call() is invoked from on_grpc_call_response(). The problem is originated by a self.grpc_callouts.borrow_mut() being invoked inside an if condition, that keeps the RefMut alive until the true branch ends.
This PR adopts the same approach than on_http_call_response(), by invoking the RefCell::borrow_mut() outside the if condition.

@andytesti andytesti requested a review from PiotrSikora as a code owner July 1, 2024 22:37
@andytesti andytesti force-pushed the fix_grpc_borrow_mut branch from 1c6bb0b to 6991ce7 Compare July 1, 2024 22:40
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a great find (and something that we've missed in #56, EDIT: because it predates gRPC support added in #100 and #101).

However, this fix is incomplete, since it only covers on_grpc_receive() callback, and only for gRPC calls and not for gRPC streams.

Could you also fix the case for nested gRPC streams in on_grpc_receive() and for nested gRPC calls and streams in on_grpc_close()?

@andytesti andytesti force-pushed the fix_grpc_borrow_mut branch 2 times, most recently from 4606852 to cd53a9e Compare July 3, 2024 14:35
@andytesti
Copy link
Contributor Author

Thanks! This is a great find (and something that we've missed in #56, EDIT: because it predates gRPC support added in #100 and #101).

However, this fix is incomplete, since it only covers on_grpc_receive() callback, and only for gRPC calls and not for gRPC streams.

Could you also fix the case for nested gRPC streams in on_grpc_receive() and for nested gRPC calls and streams in on_grpc_close()?

Thanks for reviewing this, @PiotrSikora. I just added fixes for nested gRPC streams and on_grpc_close() cases.

@andytesti andytesti requested a review from PiotrSikora July 3, 2024 15:08
@andytesti andytesti force-pushed the fix_grpc_borrow_mut branch 2 times, most recently from d488c1f to 8429900 Compare July 6, 2024 19:33
…_stream() from within gRPC event listeners.

Signed-off-by: andytesti <atesti@salesforce.com>
@andytesti andytesti force-pushed the fix_grpc_borrow_mut branch from 8429900 to 3085e79 Compare July 6, 2024 19:35
@PiotrSikora PiotrSikora changed the title Fix BorrowMutError panic when invoking dispatch_grpc_call() from on_grpc_call_response() Add support for nested gRPC callouts. Jul 13, 2024
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@PiotrSikora PiotrSikora merged commit ec3ddd2 into proxy-wasm:main Jul 19, 2024
18 checks passed
antonengelhardt pushed a commit to antonengelhardt/proxy-wasm-rust-sdk that referenced this pull request Oct 23, 2024
Signed-off-by: andytesti <atesti@salesforce.com>
mswaagman-godaddy pushed a commit to mswaagman-godaddy/proxy-wasm-rust-sdk that referenced this pull request Nov 27, 2024
Signed-off-by: andytesti <atesti@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants