reactor: deprecate at_destroy(), at_exit() #2642
Conversation
include/seastar/core/reactor.hh
Outdated
| @@ -526,10 +531,16 @@ public: | |||
| void at_exit(noncopyable_function<future<> ()> func); | |||
|
|
|||
| template <typename Func> | |||
| [[deprecated("Use app_template::run() for orderly shutdown")]] | |||
There was a problem hiding this comment.
Why app_template::run(), shouldn't it be engine().at_exit() instead?
There was a problem hiding this comment.
This patch is deprecating reactor::at_exit().
app_template::run offers a way to destroy stuff in reverse order of construction, unlike at_exit.
There was a problem hiding this comment.
This patch is deprecating reactor::at_exit().
This patch is deprecating reactor::at_destroy(), reactor::at_exit() remains intact
app_template::run offers a way to destroy stuff in reverse order of construction, unlike at_exit.
Can you point to corresponding demo/doc/test that shows how to do it? (here, not in deprecation message)
There was a problem hiding this comment.
This patch is deprecating reactor::at_exit().
This patch is deprecating reactor::at_destroy(), reactor::at_exit() remains intact
Right. I don't even know what the differences are. Should we deprecate them both?
app_template::run offers a way to destroy stuff in reverse order of construction, unlike at_exit.
Can you point to corresponding demo/doc/test that shows how to do it? (here, not in deprecation message)
sharded<foo> thing;
thing.start().get();
auto undo_thing = defer([] { thing.stop().get(); });There was a problem hiding this comment.
Right. I don't even know what the differences are.
The at_exit() tasks are run while reactor is being stopped, while at_destroy() ones -- after it is stopped.
Quoting 6ad9114
reactor: add at_destroy() function to the reactor and use it Unfortunately at_exit() cannot be used to delete objects since when it runs the reactor is still active and deleted object may still been used. We need another API that runs its task after reactor is already stopped. at_destroy() will be such api.
Should we deprecate them both?
Less API the better, I second that
sharded<foo> thing; thing.start().get(); auto undo_thing = defer([] { thing.stop().get(); });
Ah. Then I double my seconding of at_exit() deprecation. There are traces of at_exit() usage in Scylla main, but it anyway switched to this approach.
However the "Use app_template::run() for orderly shutdown" message is confusing :( How about "Use defer-s in app_template::run() async lambda for orderly shutdown" ? And the example above is definitely worth being added to the patch comment
There was a problem hiding this comment.
Well, if the message confused you, anyone would be confused, I'll improve it.
avikivity
force-pushed
the
deprecate-at_destroy
branch
from
c66b32a to
6514fc8
Compare
avikivity
changed the title
at_destroy() tasks are unordered with respect to each other, so are not very useful. They also consume a scheduling group. Since they are public, we can't remove them outright, but prepare by deprecating the public interface. Internal users are converted to a private interface. An explanation about how to use seastar::app_template is added to the documentation.
at_exit() tasks are unordered with respect to each other, so are not very useful. Since they are public, we can't remove them outright, but prepare by deprecating the public interface. Internal users are converted to a private interface. An explanation about how to use seastar::app_template is added to the documentation.
avikivity
force-pushed
the
deprecate-at_destroy
branch
from
6514fc8 to
549343a
Compare
|
v2:
|
at_destroy() tasks are unordered with respect to each other, so are not very useful. They also consume a scheduling group. at_exit() functions are similar.
Since they are public, we can't remove them outright, but prepare by deprecating the public interface. Internal users are converted to a private interface.
We add a section to the tutorial to explain how modern initialization and cleanup are done.