Skip to content
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

refactor: shutdown signaling in ping-pong #284

Merged
merged 5 commits into from Sep 28, 2023
Merged

Conversation

hopeyen
Copy link
Collaborator

@hopeyen hopeyen commented Sep 23, 2023

Description

  • Trying out the simplest workflow of shutdown signaling control for mutli-thread in the ping-pong example's context. The similar workflow can be used in other radios along with more considerations such as http service, metric port, and/or db connection.
  • Refactored register_handler and message channel sender into sdk such that a WakuMessage mpsc is required and thus register_handler becomes generic and can be automatically called when initiating a Graphcast agent.
  • Need to increment SDK's minor version

Issue link (if applicable)

Part of https://github.com/graphops/engineering-meta/issues/43

Checklist

  • Are tests up-to-date with the new changes? (no unit tests, can add in e2e)
  • Are docs up-to-date with the new changes? (update ping-pong tutorial)

@hopeyen hopeyen added the p1 High priority label Sep 23, 2023
@hopeyen hopeyen self-assigned this Sep 23, 2023
@axiomatic-aardvark
Copy link
Collaborator

axiomatic-aardvark commented Sep 27, 2023

It's kind of hard to test this when waku-org/waku-rust-bindings#66 is not resolved since it often causes early shutdown of the Radio.

Overall it looks like it's working, if the Radio starts running properly and I do ctrl+c it does shut everything down. Although sometimes there's a large interval between clicking ctrl+c and the actual shut down (something like 30 seconds). Also the logs keep going after the "Shutting down" log:
image
So to recap - sometimes it shuts down fast, sometimes it takes a while and the logs keep going, then it shuts down.

Something weird also happens sometimes when there's a panic (from the 'sni' issue)

Screenshot 2023-09-27 at 9 53 24

I click ctrl+c but it doesn't to anything and I have to close the terminal.

Again - sometimes it panics and shuts down, other times it gets stuck.

Overall it's still a great improvement though! The main purpose of the PR (to make the program stop after a shutdown signal) is delivered, I think it can go in and we can have a new Issue to tackle why it sometimes doesn't shut down when there's a panic, and one for the sometimes observed delay + logs that keep going after a shutdown signal, before the actual shutdown. Up to you, either that or those two things can be tackled as part of this PR.

@hopeyen
Copy link
Collaborator Author

hopeyen commented Sep 27, 2023

Thanks for the review. That sounds good, I will let this one set for a bit and open up a different PR for updating the waku rust binding version and see if that fixes the 'sni' issue for the radios to not panic from that

@hopeyen
Copy link
Collaborator Author

hopeyen commented Sep 27, 2023

While issue sni gets fixed in dependency, I've added a force shutdown that allows 5 seconds of wait time for graceful shutdown for ping-pong example; can be adjusted for specific radios

@axiomatic-aardvark
Copy link
Collaborator

timeout works perfectly

@hopeyen hopeyen merged commit 6746c9a into dev Sep 28, 2023
2 checks passed
@hopeyen hopeyen deleted the hope/shutdown-signaling branch September 28, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants