scylladb / seastar

High performance server-side application framework
http://seastar.io
Apache License 2.0
8.31k stars 1.54k forks source link

Seastar shouldn't always traps SIGINT and SIGTERM #261

Open nyh opened 7 years ago

nyh commented 7 years ago

I already raised this issue several months ago on the mailing list (in a thread titled "How do we handle SIGINT?" with various comments from several Seastar developers) but creating an issue now so we don't forget it.

If you try one of the network server examples in the tutorial, one thing you'll quickly realize is that it's hard to kill this server: control-C (SIGINT) is ignored, and so is "kill" with the default SIGTERM (obviously, "kill -9" does work). This is annoying: It's good that Seastar allows you to capture SIGINT et al. and turn them into asynchronous events that work safely with the rest of the Seastar programming module, but this should be an optional feature, and code that did not explicitly ask to capture these events should probably let them retain the default behavior of killing the program.

303248153 commented 6 years ago

Is anyone looking at this problem? I found replace "app.run" with "app.run_deprecated" will solve this problem, because "app.run" will register an exit function and waiting for the first future resolved. But I think "app.run" does the right thing, just I don't know is there a clean way to make "seastar::keep_doing" stop after SIGINT is received.

Also I found the example apps. like memcached and httpd, uses run_deprecated, this is a bit awkward...

tgrabiec commented 6 years ago

@303248153 You can stop your loops like this:

bool stopped = false;
return app.run(argc, argv, [&] {
    engine().at_exit([&] {
        stopped = true;
        return make_ready_future();
    });
    return do_until([&] { return stopped; }, [] {
          ... // main loop
    });
});
nyh commented 6 years ago

There are two issues here:

  1. When SIGINT / SIGTERM is caught, it needs to activate some sort of notification mechanism which the application can use to trigger its clean shutdown. Apparently there is already such mechanism as @tgrabiec mentioned in the previous comment, but this mechanism needs to be explained in the tutorial.
  2. By default, if the application does not specifically use the above mechanism to handle SIGINT / SIGTERM, Seastar shouldn't capture this signal at all, and it should trigger an immediate exit - because this is the Unix/Linux tradition. And because it's good enough for many applications (and certainly for test applications).
avikivity commented 6 years ago

We should really change those APIs.

Something like

    bool stopped = false;
    // FIXME: do_with or a seastar::thread
    seastar::signal_handler sh(SIGTERM, [&] { stopped = true; });
    return do_until([&] { return stopped; }, [] {
        ... // main loop
    });

And now we're no longer dependent on an exit concept, it works just as well with SIGHUP or anything else.