-
Notifications
You must be signed in to change notification settings - Fork 541
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
feat: implements Promise for dispatch callback #1377
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1377 +/- ##
==========================================
+ Coverage 35.91% 43.93% +8.02%
==========================================
Files 69 76 +7
Lines 11576 9965 -1611
==========================================
+ Hits 4157 4378 +221
+ Misses 7104 5253 -1851
- Partials 315 334 +19 |
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.
Let's integrate the functionality in this PR's plugin wrapper further. Hopefully, we can complete the httpcall feature for rust in this PR. For users, it should be very simple to use
fn on_http_call_response( | ||
&mut self, | ||
_token_id: u32, | ||
_num_headers: usize, | ||
_body_size: usize, | ||
_num_trailers: usize, | ||
) { | ||
self.http_dispatcher | ||
.callback(_token_id, _num_headers, _body_size, _num_trailers) | ||
} | ||
} |
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.
This logic should be placed on the SDK side, so the user's code doesn't need to worry about this detail
pub fn dispatch( | ||
&mut self, | ||
upstream: &str, | ||
headers: Vec<(&str, &str)>, | ||
body: Option<&[u8]>, | ||
trailers: Vec<(&str, &str)>, | ||
timeout: Duration, | ||
) -> Rc<Promise<(u32, usize, usize, usize)>> { |
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.
Based on this, further encapsulation is needed, such as supporting the extraction of domain and path from URL and cluster, as well as supporting get/post, etc
cc @007gzs |
if let Some(body) = hostcalls::get_http_call_response_body(0, _body_size) { | ||
if !body.is_empty() && body[0] % 2 == 0 { | ||
log.info("Access granted."); | ||
hostcalls::resume_http_request(); |
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.
必须直接调用hostcalls对普通用户是不是不太友好
Ⅰ. Describe what this PR did
In the current envoy WASM plugins, if we need IO request such as HTTP/GRPC/Redis, we must register request to envoy event loop and assigned a token via such as
dispatch_http_call
and WASM plugin should yield current request lifetime usingAction::Pause
. When the IO request completed, the envoy will callback to WASM plugin viaon_http_call_response
, and plugin should dispatch response using token (or ignored if single IO request). If we want to share something betweendispatch_http_call
andon_http_call_response
, we must share them in plugin context fields, it's not a suitable scope.Here is an example from proxy-wasm-rust-sdk
So there is three major problem we need to resolve:
In Rust async programming, normally we use
async/await
for IO request, but in envoy WASM plugin, there is no executor to poll future. A suitable solution is providing JavaScript style Promise (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise). With Promise, we could write all logic in single functionIt seems more fluid then writing in callback of
on_http_call_response
, but there is no executor for promise to trigger state transferring. We can useon_http_call_response
as trigger simplyAs for making relationship between multi tokens and promises, we could just using
HashMap
withinsert/remove
(maybe it should be embed in SDK but not belongs to this PR)Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews