Skip to content

Add support for nested gRPC callouts.#240

Merged
PiotrSikora merged 3 commits intoproxy-wasm:mainfrom
andytesti:fix_grpc_borrow_mut
Jul 19, 2024
Merged

Add support for nested gRPC callouts.#240
PiotrSikora merged 3 commits intoproxy-wasm:mainfrom
andytesti:fix_grpc_borrow_mut

Conversation

@andytesti
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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