Add seastar signal api #2384

tomershafir · 2024-08-04T07:00:41Z

fixes: #261.

The PR adds an object based interface for signals on seastar namespace.

Adds signal.hh header implemented by reactor.cc.
Adds doc/signal.md.
Deprecates reactor::handle_signal.
Supports C++20 modules.

Future possibilities:

Add signal handler unset logic, and unset signal handler on destructor.
Consider to add a reliable test case for once (should require reactor changes to expose an indicator).

xemul · 2024-08-07T06:29:06Z

@tomershafir , please fix the commits sequence, it contains some "Merge branch 'master'" and a post-fixlet, both are not welcome

xemul · 2024-08-07T06:53:09Z

doc/signal.md

+
+You need to construct a `signal_handler` instance that will register the provided signal handler for the specified signal based on the configuration params.
+
+At the moment, the signal handler will remain past object destruction.


That's extremely confusing. I would expect that desctruction of an object unregisters the handler

Ill add unregister logic to this PR

xemul · 2024-08-07T06:54:07Z

include/seastar/core/reactor.hh

    void handle_signal(int signo, noncopyable_function<void ()>&& handler);
    void wakeup();
    /// @private
    bool stopped() const noexcept { return _stopped; }

 private:
    class signals {
+        struct _signal_handler {


Leading underscore is for private members, class names shouldn't have it

xemul · 2024-08-07T06:54:39Z

include/seastar/core/reactor.hh

    friend class thread_pool;
    friend class thread_context;
    friend class internal::cpu_stall_detector;
+    friend struct signal_handler;


New friends for reactor class are not welcome. It's large enough already

The other ways would be:

Add public reactor::handle_signal_once and undo deprecation of reactor::handle_signal. Users will be able to call them and I think it inverts the purpose of changing the api. Also, it adds more code to the reactor.

Refactor signal logic out of the reactor. It seems like the blast radius is big and it's not an easy task.

Thus I would prefer the friend struct solution at the moment. Let me know wdyt

We already have nested class signal, public nested class signal_handler would be nice replacement for explicit friend. But as I wrote above, installing handler in constructor smells like RAII and implies unsetting it in destructor

My goal has been to add a clean seastar namespace level api, not through the reactor.

tomershafir · 2024-08-11T13:22:57Z

@xemul Regarding destructor logic, it seems to me like the RAII approach here may be less suitable. As the POSIX api doesn't fit RAII naturally, adapting it can introduce complexity and confusion. I assume that multiple objects can be constructed for a single signo, like POSIX.

For example:

seastar::signal_handler signal_handler1(SIGINT, [this] { signaled(); });
...
seastar::signal_handler signal_handler2(SIGINT, [this] { signaled(); });
...

What should happen if signal_handler1 is destroyed after signal_handler2 is constructed and before it's destroyed?
What should happen if signal_handler1 is destroyed after signal_handler2 is destroyed?
The possible solutions to these problems don't seem natural for RAII to me.

I think I should re-target this PR to introduce a clean seastar namespace level api, and do it via procedures rather than objects. Wdyt?

xemul · 2024-08-15T07:36:00Z

I think I should re-target this PR to introduce a clean seastar namespace level api, and do it via procedures rather than objects. Wdyt?

Yes, this sounds good

tomershafir · 2024-08-18T14:23:56Z

@xemul I force pushed the changes

xemul · 2024-08-21T13:46:59Z

tests/unit/signal_test.cc

@@ -20,6 +20,7 @@
 */

 #include <seastar/core/reactor.hh>


Is this header still needed?

No, will remove

xemul · 2024-08-21T13:48:16Z

include/seastar/core/reactor.hh

    signals _signals;
    std::unique_ptr<thread_pool> _thread_pool;
+


Unrelated change

xemul · 2024-08-21T13:48:27Z

include/seastar/core/reactor.hh

    };
-
+    


Unwanted indentation

xemul · 2024-08-21T13:48:38Z

include/seastar/core/reactor.hh

-            noncopyable_function<void ()> _handler;
-        };
-        std::atomic<uint64_t> _pending_signals;
-        std::unordered_map<int, signal_handler> _signal_handlers;


Why is this block moved upwards?

To remove redundant private in a class

Ill extract it into a different PR

xemul · 2024-08-21T13:50:49Z

src/core/signal.cc

+namespace seastar {
+
+static reactor& engine_or_throw() {
+    if (!engine_is_ready()) {


I think that's impossible. No tasks can be run until engine is ready

Its possible, if a user calls the procedure before app.run(), like:

seastar::handle_signal(SIGINT, [] {}); return app.run(argc, argv, [&app] -> seastar::future<> { ...

I dont think its obvious that it must be called inside run(), so I think the logic provides a more controlled and understandable error handling.

In this case the "reactor must be initialized" can be confusing message, because it's not obvious that "app.run()" initializes the reactor. I would just mention in the method doc, that it's only valid to use one from inside app.run() lambda, otherwise it's UB

xemul · 2024-08-26T08:20:01Z

src/core/signal.cc

@@ -31,15 +31,8 @@ module seastar;

 namespace seastar {

-static reactor& engine_or_throw() {


Please, squash it with the previous commit

sorry, done

tomershafir · 2024-08-30T06:59:39Z

@xemul close #261?

Closes scylladb#2384

tomershafir marked this pull request as ready for review August 4, 2024 09:25

tomershafir force-pushed the add-seastar-signal-api branch 2 times, most recently from f9bdcc2 to 1834cb5 Compare August 7, 2024 06:47

xemul reviewed Aug 7, 2024

View reviewed changes

tomershafir force-pushed the add-seastar-signal-api branch from 1834cb5 to 6156b3e Compare August 18, 2024 14:04

xemul reviewed Aug 21, 2024

View reviewed changes

include/seastar/core/reactor.hh Outdated

};

Copy link

Contributor

xemul Aug 21, 2024

Choose a reason for hiding this comment

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

Unwanted indentation

tomershafir reacted with thumbs up emoji

xemul reviewed Aug 21, 2024

View reviewed changes

tomershafir force-pushed the add-seastar-signal-api branch 2 times, most recently from 795134e to 01aa880 Compare August 22, 2024 13:26

tomershafir requested a review from xemul August 22, 2024 13:42

xemul reviewed Aug 26, 2024

View reviewed changes

signal: add seastar signal api

9e16998

tomershafir force-pushed the add-seastar-signal-api branch from b5d1f22 to 9e16998 Compare August 26, 2024 11:47

xemul closed this in c3e826a Aug 30, 2024

tomershafir deleted the add-seastar-signal-api branch August 30, 2024 06:59

niekbouman pushed a commit to niekbouman/seastar that referenced this pull request Sep 16, 2024

signal: add seastar signal api

9d0d323

Closes scylladb#2384

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add seastar signal api #2384

Add seastar signal api #2384

tomershafir commented Aug 4, 2024 •

edited

Loading

xemul commented Aug 7, 2024

xemul Aug 7, 2024

tomershafir Aug 7, 2024

xemul Aug 7, 2024

xemul Aug 7, 2024

tomershafir Aug 7, 2024

xemul Aug 7, 2024

tomershafir Aug 7, 2024

tomershafir commented Aug 11, 2024

xemul commented Aug 15, 2024

tomershafir commented Aug 18, 2024

xemul Aug 21, 2024

tomershafir Aug 21, 2024

xemul Aug 21, 2024

xemul Aug 21, 2024

xemul Aug 21, 2024

tomershafir Aug 21, 2024

tomershafir Aug 21, 2024

xemul Aug 21, 2024

tomershafir Aug 22, 2024

xemul Aug 22, 2024

xemul Aug 26, 2024

tomershafir Aug 26, 2024

tomershafir commented Aug 30, 2024


		You need to construct a `signal_handler` instance that will register the provided signal handler for the specified signal based on the configuration params.

		At the moment, the signal handler will remain past object destruction.

		@@ -31,15 +31,8 @@ module seastar;

		namespace seastar {

		static reactor& engine_or_throw() {

Add seastar signal api #2384

Add seastar signal api #2384

Conversation

tomershafir commented Aug 4, 2024 • edited Loading

xemul commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomershafir commented Aug 11, 2024

xemul commented Aug 15, 2024

tomershafir commented Aug 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomershafir commented Aug 30, 2024

tomershafir commented Aug 4, 2024 •

edited

Loading