Replace signal() with sigaction() and add signal flags support #48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis PR implements POSIX signal flags support for Changes
Sequence DiagramsequenceDiagram
participant Caller as Application Code
participant API as signal_set::add<br/>(int, flags_t)
participant Validate as flags_supported<br/>flags_compatible
participant State as signal_state<br/>registered_flags
participant Register as posix_signals<br/>::add_signal
participant SA as sigaction()<br/>kernel
Caller->>API: add(SIGTERM, restart)
API->>Validate: flags_supported(restart)
alt Flags invalid
Validate-->>API: false
API-->>Caller: error
end
API->>State: Check registered_flags[SIGTERM]
alt First registration
State-->>API: (empty/none)
else Re-registration
State->>Validate: flags_compatible(existing, restart)
alt Incompatible flags
Validate-->>API: false
API-->>Caller: error
end
end
API->>Register: add_signal(impl, SIGTERM, restart)
Register->>Validate: to_sigaction_flags(restart)
Validate-->>Register: SA_RESTART
Register->>SA: sigaction(SIGTERM, &sa{flags=SA_RESTART})
SA-->>Register: success
Register->>State: registered_flags[SIGTERM] = restart
Register-->>API: success
API-->>Caller: result<void>()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@include/boost/corosio/signal_set.hpp`:
- Around line 92-94: Update the doc comment in signal_set.hpp (the note near the
signal_set/flags description) to reflect actual Windows behavior: do not say
flags are "silently ignored"—state that on Windows the backend returns
operation_not_supported (error) for any flag other than none/dont_care; mention
that flags are only meaningful on POSIX and that callers should expect an
operation_not_supported error on Windows when passing unsupported flags. Refer
to signal_set and its flags in the comment so readers can locate the behavior.
In `@src/corosio/src/detail/posix/signals.cpp`:
- Around line 135-138: The code currently ignores requests for
signal_set::no_child_wait when SA_NOCLDWAIT is not defined; modify the handling
in src/corosio/src/detail/posix/signals.cpp so the caller is not misled: either
(A) add a compile-time guard that fails fast when signal_set::no_child_wait is
used but SA_NOCLDWAIT is unavailable (e.g. detect flags &
signal_set::no_child_wait and return a sentinel/error), or (B) emit a clear
runtime warning/error and return a failure code when flags contains
signal_set::no_child_wait but SA_NOCLDWAIT is undefined; update the caller
contract (function that consumes flags/returns sa_flags) to propagate this error
so callers can detect unsupported no_child_wait.
- Around line 75-77: deliver_signal() runs in a signal handler but currently
acquires non-async-signal-safe locks (e.g., std::mutex), which is undefined
behavior; change the implementation so the handler does not use mutexes: make
deliver_signal() perform only async-signal-safe actions (set a sig_atomic_t flag
or write a byte to a self-pipe/eventfd) and move the heavy work and any locking
into the main/event loop where handle_pending_signals() (or similar consumer
code) reads the flag/pipe and then acquires mutexes; update sigaction() setup
comments and/or document the new restriction if you choose a minimal change.
In `@src/corosio/src/detail/win/signals.hpp`:
- Around line 42-45: Update the comment in signals.hpp so it accurately
describes runtime behavior: replace "Signal flags are accepted but silently
ignored" with a note that unsupported signal flags are rejected at runtime with
operation_not_supported (or equivalent error), and mention that callers should
expect EOPNOTSUPP/operation_not_supported rather than silent no-op; ensure the
revised comment references the Windows-specific behavior already noted (use of C
runtime signal() and re-registration requirement) so maintainers see the
complete platform limitations.
🧹 Nitpick comments (1)
src/corosio/src/detail/posix/signals.cpp (1)
403-407: Useerrnofor accurate error reporting fromsigaction().When
sigaction()fails, the code returns a genericinvalid_argumenterror. The actual failure reason is inerrnoand could be more informative (e.g.,EINVALfor invalid signal number,EFAULTfor bad pointer).Suggested improvement
if (::sigaction(signal_number, &sa, nullptr) < 0) { delete new_reg; - return make_error_code(system::errc::invalid_argument); + return make_error_code(static_cast<system::errc>(errno)); }
sgerbino
force-pushed
the
feature/sigaction
branch
from
fbe4889 to
3fbc0c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@doc/asio-feature-parity.md`:
- Around line 152-160: Update the Signal Handling table to reflect the new
corosio::signal_set implementation: change the Corosio column entries for
`signal_set`, `async_wait` for signals, and `Add/remove signals` from :x: to
:white_check_mark: (or a partial/conditional mark if you prefer), and update the
Notes cell to mention that corosio::signal_set is flag-aware and implemented via
sigaction() on POSIX, with Windows limitations (e.g., limited/partial support on
Windows). Specifically reference `corosio::signal_set` and mention the use of
`sigaction()` and the platform caveat in the Notes column.
In `@doc/epoll-for-dummies.md`:
- Around line 20-35: The fenced code/diagram blocks in the doc (for example the
ASCII diagram shown inside the triple backticks in the posix_scheduler diagram)
are missing language identifiers and trigger MD040; update each triple-backtick
fence to include an appropriate language tag (use text for ASCII diagrams and
cpp for C++ code examples) so markdownlint passes—apply this change to the
blocks around the shown snippet and the other ranges called out (71-110,
196-205, 240-262, 315-353, 481-507, 609-614, 625-637, 646-670, 762-798).
- Around line 371-378: The doc snippet shows corosio_posix_signal_handler
calling posix_signals::deliver_signal and then re-registering via
::signal(signal_number, corosio_posix_signal_handler); update it to reflect the
sigaction-based implementation by removing the re-registration line and noting
that sigaction persists the handler automatically (or briefly mention using
sigaction instead of signal), keeping corosio_posix_signal_handler and
posix_signals::deliver_signal references intact.
In `@doc/issues/sigaction-support.md`:
- Around line 148-149: Replace the two bare URLs by turning them into Markdown
inline links to satisfy MD034: change the lines containing "Boost.Asio
`signal_set`" and the Boost URL to a linked form like [Boost.Asio
`signal_set`](https://www.boost.org/doc/libs/release/doc/html/boost_asio/reference/signal_set.html)
and change the line containing "POSIX sigaction" and the POSIX URL to [POSIX
sigaction](https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html);
ensure the visible text remains the descriptive labels (`signal_set` and
`sigaction`) so the markdownlint no-bare-urls rule is satisfied.
♻️ Duplicate comments (1)
src/corosio/src/detail/win/signals.hpp (1)
180-185: Alignadd_signalparam docs with the runtime flag validation.The param note says flags are ignored, but the header block says unsupported flags return
operation_not_supported. Aligning these avoids mixed guidance.📝 Suggested doc tweak
- `@param` flags The flags to apply (ignored on Windows). + `@param` flags Only `none` and `dont_care` are supported; other flags + return `operation_not_supported`.
🧹 Nitpick comments (2)
doc/issues/cpp-modules-support.md (1)
45-55: Add a language tag to the fenced block.MD040: the fence at Line 45 has no language; add one (e.g.,
text) to satisfy markdownlint.✅ Proposed fix
-``` +```text boost.corosio - Primary module interface boost.corosio:io_context - io_context and scheduler boost.corosio:socket - Socket types boost.corosio:acceptor - TCP acceptor boost.corosio:resolver - DNS resolver boost.corosio:read - Read operations boost.corosio:write - Write operations boost.corosio:tls - TLS stream (WolfSSL) boost.corosio:buffers - Buffer types (any_bufref, consuming_buffers)doc/issues/epollexclusive-thundering-herd.md (1)
50-50: Replace hardcoded line numbers with function/section references.The document references specific line numbers (
scheduler.cpp:94,sockets.hpp:462,doc/epoll-for-dummies.mdlines 621-643) which will become stale as the codebase evolves. Consider referencing by function name or section header instead.For example:
- Line 50: "registered in
scheduler::initialize_epoll()" instead of "scheduler.cpp:94"- Line 110: "in
socket::async_accept()" instead of "sockets.hpp:462"♻️ Proposed changes
-The eventfd used for wakeup signaling is registered at `scheduler.cpp:94`: +The eventfd used for wakeup signaling is registered in the scheduler initialization:-`EPOLLEXCLUSIVE` is also relevant for accept operations on listening sockets (`sockets.hpp:462`). When multiple threads wait to accept on the same socket, `EPOLLEXCLUSIVE` prevents all threads from waking on each incoming connection. +`EPOLLEXCLUSIVE` is also relevant for accept operations on listening sockets. When multiple threads wait to accept on the same socket, `EPOLLEXCLUSIVE` prevents all threads from waking on each incoming connection.-- Corosio documentation: `doc/epoll-for-dummies.md` (lines 621-643) +- Corosio documentation: `doc/epoll-for-dummies.md` (section on multi-threaded epoll usage)Also applies to: 110-110, 184-184
sgerbino
force-pushed
the
feature/sigaction
branch
from
3fbc0c4 to
7411881
Compare
Replace the POSIX signal() implementation with sigaction() and add support for signal flags (SA_RESTART, SA_NOCLDSTOP, etc.) while keeping OS-specific headers out of the public API. Public API changes: - Add flags_t enum directly to signal_set with abstract bit values - Add signal_set::add(int, flags_t) overload - Existing add(int) becomes inline convenience calling add(signal, none) - Add bitwise operators for flag manipulation (|, &, |=, &=, ~) Available flags: none, restart, no_child_stop, no_child_wait, no_defer, reset_handler, dont_care POSIX implementation (posix/signals.cpp): - Replace signal() with sigaction() for robust signal handling - Add flags_supported() to validate flags available on this platform - Add to_sigaction_flags() to map abstract flags to SA_* constants - Add flag conflict detection for multiple signal_sets on same signal - Remove handler re-registration (not needed with sigaction) - Track registered flags in global signal_state for cross-set validation - Return operation_not_supported if SA_NOCLDWAIT unavailable - Document async-signal-safety limitation in signal handler Windows implementation (win/signals.cpp): - Only none and dont_care flags are supported - Other flags return operation_not_supported Testing: Cross-platform tests for none/dont_care, POSIX-only tests for actual flag behavior, Windows-only test for operation_not_supported.
sgerbino
force-pushed
the
feature/sigaction
branch
from
7411881 to
14edeba
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace signal() with sigaction() and add signal flags support
Replace the POSIX signal() implementation with sigaction() and add
support for signal flags (SA_RESTART, SA_NOCLDSTOP, etc.) while
keeping OS-specific headers out of the public API.
Public API changes:
Available flags: none, restart, no_child_stop, no_child_wait, no_defer,
reset_handler, dont_care
POSIX implementation (posix/signals.cpp):
Windows implementation (win/signals.cpp):
Testing: Cross-platform tests for none/dont_care, POSIX-only tests for
actual flag behavior, Windows-only test for operation_not_supported.
Resolves #36.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.