Skip to content

improvement(core): handle more OS signals and broadcast them#2667

Merged
ca333 merged 5 commits intodevfrom
handle-more-signals
Oct 27, 2025
Merged

improvement(core): handle more OS signals and broadcast them#2667
ca333 merged 5 commits intodevfrom
handle-more-signals

Conversation

@onur-ozkan
Copy link
Copy Markdown

@onur-ozkan onur-ozkan commented Oct 27, 2025

Makes KDF to broadcast (through streaming manager) OS signal before the graceful shutdown.

Also, refactors the spawn_ctrl_c_handler function to cover more signals in a more generic way (can handle more signals easily when needed).

The event activation RPC:

curl --url "http://127.0.0.1:7783" --data '{
  "userpass":"'$userpass'",
  "method":"stream::shutdown_signal::enable",
  "mmrpc":"2.0",
  "params": {},
  "id": "'$client_id'"
}'

The broadcasted message:

image

for the signals we can't gracefully shutdown, the message will be "UNKNOWN($id_of_the_signal)".

This feature isn't supported on Windows, and doesn't run on Web for obvious reasons.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
dimxy
dimxy previously approved these changes Oct 27, 2025
Copy link
Copy Markdown
Collaborator

@dimxy dimxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@onur-ozkan onur-ozkan changed the title improvement(core): handle more OS signals improvement(core): handle more OS signals and broadcast them Oct 27, 2025
@onur-ozkan
Copy link
Copy Markdown
Author

There is a new feature since your approval. @dimxy

Signed-off-by: Onur Özkan <work@onurozkan.dev>
mariocynicys
mariocynicys previously approved these changes Oct 27, 2025
Copy link
Copy Markdown
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, all comments below are nits.
LGTM!

Comment on lines +241 to +246
let signal_name = match signal {
libc::SIGINT => "SIGINT".to_owned(),
libc::SIGTERM => "SIGTERM".to_owned(),
libc::SIGQUIT => "SIGQUIT".to_owned(),
_ => format!("UNKNOWN({signal})"),
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we intend to handle any signal, shouldn't we init our signal hook with all signals (if there is a catch all thing).

not really a problem since we have the important signals covered, but looking at this, the unknown signal line would never be triggered (though i would prefer it like that instead of unreachable! so i don't have a problem with that either).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we intend to handle any signal

No, not any signal. We can't handle every signal (e.g., we can't handle SIGKILL). I wanted to handle ones (the obvious ones) that can be handled.

@CharlVS
Copy link
Copy Markdown

CharlVS commented Oct 27, 2025

@onur-ozkan thanks for this! It will make a significant difference in the GUI needing to poll to get wallet status.

This feature isn't supported on Windows, and doesn't run on Web for obvious reasons.

I'm assuming your streaming event was intended as a QoL addition to the existing PR, initially fixing an iOS-specific issue, but it is worth adding the catching to Windows as well. We won't encounter the file descriptor limit issue on non-iOS platforms, but there may be other reasons KDF shuts down via a signal. e.g. user terminates the KDF process in the task manager.

@onur-ozkan
Copy link
Copy Markdown
Author

I'm assuming your streaming event was intended as a QoL addition to the existing PR, initially fixing an iOS-specific issue, but it is worth adding the catching to Windows as well. We won't encounter the file descriptor limit issue on non-iOS platforms, but there may be other reasons KDF shuts down via a signal. e.g. user terminates the KDF process in the task manager.

Yeah, we should support Windows too but it would require a broader work. We can handle that later (I guess when it becomes very necessary?).

Signed-off-by: Onur Özkan <work@onurozkan.dev>
@ca333 ca333 merged commit 9aa41b4 into dev Oct 27, 2025
20 of 26 checks passed
@onur-ozkan onur-ozkan deleted the handle-more-signals branch October 27, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants