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

msgq receive replaces signal handlers #640

Closed
sshane opened this issue Jan 17, 2025 · 2 comments
Closed

msgq receive replaces signal handlers #640

sshane opened this issue Jan 17, 2025 · 2 comments
Labels
enhancement New feature or request

Comments

@sshane
Copy link
Contributor

sshane commented Jan 17, 2025

This basically never allows ExitHandler to see signals, needed for commaai/openpilot#32103

msgq/msgq/impl_msgq.cc

Lines 79 to 82 in 5bb86f8

if (!non_blocking){
prev_handler_sigint = std::signal(SIGINT, sig_handler);
prev_handler_sigterm = std::signal(SIGTERM, sig_handler);
}

@sshane sshane added the enhancement New feature or request label Jan 17, 2025
@sshane
Copy link
Contributor Author

sshane commented Jan 17, 2025

@deanlee want to check this out? Can we just forward to the prev signal handlers in msgq's signal handler?

@sshane sshane changed the title msgq receive removes signal handlers msgq receive replaces signal handlers Jan 17, 2025
@sshane sshane transferred this issue from commaai/openpilot Jan 17, 2025
@deanlee
Copy link
Contributor

deanlee commented Jan 17, 2025

Forwarding signal handlers isn’t ideal because it introduces complexity and requires careful management of race conditions when restoring original handlers in multi-threaded environments.

A more robust approach would be using signal masking to block signals, as done in this PR: #616

However, we might not need to handle exit signals in MSGQSubSocket::receive() at all. We could instead delegate exit signal handling to the caller, following the approach in commaai/openpilot#32367 (closed by bot), where signal handling in params was removed entirely.

update:

Reopened the PR to move blocking read functionality to Cython: commaai/openpilot#34407

@sshane sshane closed this as completed Jan 18, 2025
@sshane sshane reopened this Jan 18, 2025
@sshane sshane closed this as completed Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants