Delay sendLocalReply till after the call to prevent rentrant calls.#432
Delay sendLocalReply till after the call to prevent rentrant calls.#432PiotrSikora merged 2 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: John Plevyak <jplevyak@gmail.com>
| DeferAfterCallActions actions(this); | ||
| if (!wasm_->on_configure_) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Those could be moved to after the check if the callback actually exist, right? Otherwise, we're wasting cycles.
(Same in most other places)
There was a problem hiding this comment.
True, but it was more obviously correct, but Done.
| return true; | ||
| } | ||
|
|
||
| void addAfterVmCallAction(std::function<void()> f) { after_vm_call_actions_.push_back(f); } |
There was a problem hiding this comment.
You're both adding at and removing from the back of the queue, which makes this LIFO. I think it should be FIFO instead.
| while (!after_vm_call_actions_.empty()) { | ||
| after_vm_call_actions_.back()(); | ||
| after_vm_call_actions_.pop_back(); | ||
| } |
There was a problem hiding this comment.
This gets recursive and the deferred functions are called indefinitely before this code has a chance to remove them from the queue. I'm not sure why, to be honest.
One way to work around this is to make the call only after removing the function from the queue, i.e.
auto f = after_vm_call_actions_.back();
after_vm_call_actions_.pop_back();
f();
but that probably only works if the is only one deferred call?
There was a problem hiding this comment.
Done.
It should be fine in the loop so long as the function to be called is removed first as you suggest.
Signed-off-by: John Plevyak <jplevyak@gmail.com>
This was previously fixed in envoyproxy#432, but got somehow lost. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This was previously fixed in envoyproxy#432, but got somehow lost. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This was previously fixed in envoyproxy/envoy-wasm#432, but got somehow lost. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
* Defer sendLocalReply() to prevent reentrant calls. This was previously fixed in envoyproxy/envoy-wasm#432, but got somehow lost. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: add John's comment. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: John Plevyak jplevyak@gmail.com