wasm: remove the shutdown callback in lifetime_notifier #36688
Conversation
Signed-off-by: Boteng Yao <boteng@google.com>
botengyao
changed the title
|
/assign @mpwarres |
|
cc @yanavlasov |
|
/wait for CI |
Signed-off-by: Boteng Yao <boteng@google.com>
…time_issue Signed-off-by: Boteng Yao <boteng@google.com>
Signed-off-by: Boteng Yao <boteng@google.com>
| server_shutdown_post_cb_ = std::move(post_cb); | ||
| } | ||
| }); | ||
| shutdown_handler_ = lifecycle_notifier.registerCallback( |
There was a problem hiding this comment.
I don't really understand this code - can you explain briefly? Why is "this" held twice directly and via weak? Why is it posting the completion to its own dispatcher without doing any work?
There was a problem hiding this comment.
Taking a deeper look, I don't have a full history for why this server_shutdown_post_cb_ is needed. The previous shutdown logic is 100% no-op since it is never registered. Weak here is going to check if this object is still alive, and then the callback can be executed. The original implementation is posting the completion_cb in the destructor leading to issues which I think the root cause is: since it is latching the completion_cb, the cb_guard will never be executed, so that the server will not shutdown until it is timeout, and then further cause other segment issues.
My current version also has a issue to bypass some callbacks by posting completion_cb which is (dispatcher_.exit()) directly. Based on the above analysis, I tend to remove the whole shutdown registration code block since it is never used, WDYT?
There was a problem hiding this comment.
I think the reason was to prolong Wasm lifetime but I agree - it's difficult to understand now why.
There was a problem hiding this comment.
+1, it seems like the only purpose is to make sure that the Wasm instance is deleted before proceeding with shutdown, but was no-op before. Removing it SGTM as well.
Signed-off-by: Boteng Yao <boteng@google.com>
botengyao
changed the title
|
/retest |
|
/assign @yanavlasov for final pass, thanks! |
|
ping @yanavlasov, thanks! |
Commit Message: wasm: remove the shutdown callback in lifetime_notifier
Additional Description:
Risk Level: low
Testing: unit test
Docs Changes:
Release Notes:
Fixes: #35882