Simplify signal_set API with result-based error handling #38

sgerbino · 2026-01-20T23:07:18Z

Replace throwing/error_code overload pairs with system::result
- Replace multiple signal constructors with variadic template
- Add error checking for signal() calls when restoring SIG_DFL
- Fix signal validation to allow signal 0

Resolves #28.

Summary by CodeRabbit

New Features
- Added variadic constructor for signal sets enabling initialization with multiple signals.
- Enhanced TLS context configuration for secure HTTPS connections.
Bug Fixes
- Improved error handling in signal operations across POSIX and Windows platforms.
Refactor
- Modernized signal API return types for explicit error reporting and better error propagation.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Replace throwing/error_code overload pairs with system::result<void> - Replace multiple signal constructors with variadic template - Add error checking for signal() calls when restoring SIG_DFL - Fix signal validation to allow signal 0

coderabbitai · 2026-01-20T23:07:34Z

📝 Walkthrough

Walkthrough

This pull request refactors the signal_set API to use system::result<void> return types instead of exception-throwing or void-returning methods with out-parameter error codes. It replaces multiple fixed constructors with a variadic template and updates both POSIX and Windows signal implementations accordingly. An HTTPS client example is also updated to configure TLS context explicitly.

Changes

Cohort / File(s)	Summary
TLS Context Configuration `example/https-client/https_client.cpp`	Creates `corosio::tls::context`, sets hostname, verify paths, and peer verification mode before passing to `wolfssl_stream` constructor. Adds public TLS context API methods: `set_hostname()`, `set_default_verify_paths()`, `set_verify_mode()`.
Signal Set Public API `include/boost/corosio/signal_set.hpp`	Replaces fixed constructors for 1-3 signals with variadic template. Converts `add()`, `remove()`, `clear()` return types to `system::result<void>`. Updates `await_suspend` to include `std::stop_token` parameter. Adds includes for `boost/system/result.hpp` and `<concepts>`.
POSIX Signal Implementation `src/corosio/src/detail/posix/signals.cpp`	Converts all signal operation methods to return `system::result<void>`. Adds explicit error handling for signal restoration failures. Tightens signal number validation. Removes direct `capy::any_coro` wrapping in posting paths. Eliminates obsolete constructors.
POSIX Signal Declarations `src/corosio/src/detail/posix/signals.hpp`	Updates method signatures for `add()`, `remove()`, `clear()` to return `system::result<void>`. Changes `signal_op::h` field type from `std::coroutine_handle<>` to `capy::any_coro`. Updates documentation.
Windows Signal Implementation `src/corosio/src/detail/win/signals.cpp`	Converts signal operation methods to return `system::result<void>`. Introduces first-error aggregation in `clear_signals()`. Removes error_code out-parameter overloads and fixed-signal constructors.
Windows Signal Declarations `src/corosio/src/detail/win/signals.hpp`	Updates method signatures for `add()`, `remove()`, `clear()` to return `system::result<void>`. Adds `boost/system/result.hpp` include. Updates documentation.
Signal Set Unit Tests `test/unit/signal_set.cpp`	Replaces error_code checks with `has_value()` and `has_error()` assertions on returned results. Adapts all signal operation calls to use new return types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

tests and more #34 — Introduces corosio::tls::context and refactors constructor signatures for wolfssl_stream and openssl_stream to accept context parameters, directly paralleling the HTTPS client example changes in this PR.
Refactor details directory structure #32 — Modifies the same POSIX signal implementation files (src/corosio/src/detail/posix/signals.{cpp,hpp}), touching signal service and scheduler wiring logic.

Poem

🐰 Signals now speak in result-based tongue,
No more exceptions when errors are sung,
Variadic templates unite the way,
API cleaned up, hooray, hooray!
From POSIX to Windows, both platforms align,
Error handling returns—a design so fine! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: replacing error handling patterns with result-based error handling in the signal_set API.
Linked Issues check	✅ Passed	All coding requirements from issue `#28` are met: dual-error function patterns replaced with system::result, variadic template constructor added, and signal validation corrected.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objectives: signal_set API refactoring, error handling improvements, and constructor simplification align with issue `#28` requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cppalliance-bot · 2026-01-20T23:12:55Z

An automated preview of the documentation is available at https://38.corosio.prtest3.cppalliance.org/index.html

If more commits are pushed to the pull request, the docs will rebuild at the same URL.

2026-01-20 23:12:54 UTC

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/corosio/src/detail/posix/signals.cpp (1)
292-337: Align remove_signal error handling with clear_signals to prevent resource leak.

remove_signal returns early when SIG_DFL restore fails (line 318–320), leaving the registration undeleted and data structures in an incomplete state. clear_signals captures the error and continues cleanup, only returning the error afterward. This inconsistency causes a resource leak in the error path of remove_signal.

Capture the signal restoration error instead of returning early, proceed with cleanup (including deleting reg), and return the error at the end to match clear_signals behavior.
♻️ Proposed adjustment
+    system::error_code first_error;
     if (state->registration_count[signal_number] == 1)
     {
         if (::signal(signal_number, SIG_DFL) == SIG_ERR)
-            return make_error_code(system::errc::invalid_argument);
+            first_error = make_error_code(system::errc::invalid_argument);
     }

     *deletion_point = reg->next_in_set;

     if (registrations_[signal_number] == reg)
         registrations_[signal_number] = reg->next_in_table;
     if (reg->prev_in_table)
         reg->prev_in_table->next_in_table = reg->next_in_table;
     if (reg->next_in_table)
         reg->next_in_table->prev_in_table = reg->prev_in_table;

     --state->registration_count[signal_number];
     --registration_count_[signal_number];

     delete reg;
+    if (first_error)
+        return first_error;
     return {};

🧹 Nitpick comments (1)

test/unit/signal_set.cpp (1)
116-138: Tests correctly updated for result-based API.

The tests properly use has_value() and has_error() to verify the new system::result<void> return types. The coverage for add/duplicate/invalid scenarios is good.

One observation: the PR mentions "Fix signal validation to allow signal 0", but there's no test case verifying that add(0) now succeeds. Consider adding a test to confirm this fix:
void
testAddSignalZero()
{
    io_context ioc;
    signal_set s(ioc);

    // Signal 0 should now be valid
    auto result = s.add(0);
    BOOST_TEST(result.has_value());
}

vinniefalco · 2026-01-20T23:19:16Z

example/https-client/https_client.cpp

+    corosio::tls::context ctx;
+    ctx.set_hostname(hostname);
+    ctx.set_default_verify_paths().value();
+    ctx.set_verify_mode(corosio::tls::verify_mode::peer).value();


This is really cool

vinniefalco · 2026-01-20T23:20:00Z

include/boost/corosio/signal_set.hpp

-        int signal_number_2,
-        int signal_number_3);
+        int signal,
+        Signals... signals)


Why even keep this though? Do we like it?

vinniefalco · 2026-01-20T23:21:57Z

src/corosio/src/detail/posix/signals.cpp

-        ::signal(signal_number, SIG_DFL);
+    {
+        if (::signal(signal_number, SIG_DFL) == SIG_ERR)
+            return make_error_code(system::errc::invalid_argument);


would this also work:

return system::errc::invalid_argument;

vinniefalco · 2026-01-20T23:22:18Z

src/corosio/src/detail/posix/signals.cpp

-            ::signal(signal_number, SIG_DFL);
+        {
+            if (::signal(signal_number, SIG_DFL) == SIG_ERR && !first_error)
+                first_error = make_error_code(system::errc::invalid_argument);


would this work

first_error = system::errc::invalid_argument;

coderabbitai bot reviewed Jan 20, 2026

View reviewed changes

vinniefalco reviewed Jan 20, 2026

View reviewed changes

vinniefalco approved these changes Jan 20, 2026

View reviewed changes

sgerbino merged commit 004fdd9 into cppalliance:develop Jan 20, 2026
14 checks passed

sgerbino deleted the pr/signal-set-api branch January 20, 2026 23:28

This was referenced Jan 21, 2026

fix compilation errors relating to the capy move of intrusive_list #45

Closed

Take a copy of capy's intrusive list as an implementation detail. #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simplify signal_set API with result-based error handling #38

Simplify signal_set API with result-based error handling #38

Uh oh!

sgerbino commented Jan 20, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 20, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

cppalliance-bot commented Jan 20, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

vinniefalco Jan 20, 2026

Uh oh!

vinniefalco Jan 20, 2026

Uh oh!

vinniefalco Jan 20, 2026

Uh oh!

vinniefalco Jan 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Simplify signal_set API with result-based error handling #38

Simplify signal_set API with result-based error handling #38

Uh oh!

Conversation

sgerbino commented Jan 20, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

cppalliance-bot commented Jan 20, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

vinniefalco Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

vinniefalco Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

vinniefalco Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

vinniefalco Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sgerbino commented Jan 20, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 20, 2026 •

edited

Loading

vinniefalco Jan 20, 2026 •

edited

Loading