crimson/asok: do not assume the order of param eval

* do not assume the order of parameter evaluation, before this change,
  we have `do_with(cn.input(), cn.output(), std::move(cn) ...)`, see
  https://en.cppreference.com/w/cpp/language/eval_order,
> side effects of the initialization of every parameter are
> indeterminately sequenced with respect to value computations and side
> effects of any other parameter.
  we cannot move `cn` out and then call its member functions. so
  introduce a struct for capturing its input and output.
* move `do_until_gate()` into `start()`, no need to check if
  gate is stopped in `safe_action`, as `sestar::do_until()` will do
  this for us.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/crimson/admin/admin_socket.cc b/src/crimson/admin/admin_socket.cc
index 9652acdba187ac6054dbfa5661b817b3bdf88ccd..57836fde1f42f2daf426b27d8a8357381e4a8646 100644 (file)
--- a/src/crimson/admin/admin_socket.cc
+++ b/src/crimson/admin/admin_socket.cc
@@ -202,25 +202,15 @@ seastar::future<> AdminSocket::handle_client(seastar::input_stream<char>& in,
   }).discard_result();
 }
 
-/**
- *  \brief continuously run a block of code "within a gate"
- *
- *  Neither gate closure, nor a failure of the code block we are running, will
- *  cause an exception to be thrown by safe_action_gate_func()
- */
-template <typename AsyncAction>
-seastar::future<> do_until_gate(seastar::gate& gt, AsyncAction action)
-{
-  auto stop_cond = [&gt] { return gt.is_closed(); };
-  auto safe_action{ [act = std::move(action), &gt]() mutable {
-    if (gt.is_closed())
-      return seastar::make_ready_future<>();
-    return with_gate(gt, [act = std::move(act)] {
-      return act().handle_exception([](auto e) {});
-    });
-  } };
-
-  return seastar::do_until(stop_cond, std::move(safe_action)).discard_result();
+namespace {
+struct connection_t {
+  seastar::connected_socket cs;
+  seastar::input_stream<char> input;
+  seastar::output_stream<char> output;
+  connection_t(seastar::connected_socket&& s) :
+    cs(std::move(s)), input(cs.input()), output(cs.output())
+  {}
+};
 }
 
 seastar::future<> AdminSocket::start(const std::string& path)
@@ -233,53 +223,38 @@ seastar::future<> AdminSocket::start(const std::string& path)
 
   logger().debug("{}: asok socket path={}", __func__, path);
   auto sock_path = seastar::socket_address{ seastar::unix_domain_addr{ path } };
-
-  std::ignore = return seastar::do_with(
-      seastar::engine().listen(sock_path),
-      [this](seastar::server_socket& lstn) {
-        m_server_sock = &lstn;  // used for 'abort_accept()'
-        return do_until_gate(arrivals_gate, [&lstn, this] {
-          return lstn.accept().then(
-            [this](seastar::accept_result from_accept) {
-              seastar::connected_socket cn =
-               std::move(from_accept.connection);
-             return do_with(cn.input(), cn.output(), std::move(cn),
-                [this](auto& inp, auto& out, auto& cn) {
-                 return handle_client(inp, out).finally([] {
-                   ;  // left for debugging
-                  });
-               });
-           });
-          }).then([] {
-            logger().debug("AdminSocket::init(): admin-sock thread terminated");
-            return seastar::now();
+  server_sock = seastar::engine().listen(sock_path);
+  // listen in background
+  std::ignore = seastar::do_until(
+    [this] { return stop_gate.is_closed(); },
+    [this] {
+      return seastar::with_gate(stop_gate, [this] {
+        return server_sock->accept().then([this](seastar::accept_result acc) {
+          return seastar::do_with(connection_t{std::move(acc.connection)},
+            [this](auto& conn) mutable {
+            return handle_client(conn.input, conn.output);
           });
+        }).handle_exception([this](auto ep) {
+          if (!stop_gate.is_closed()) {
+            logger().error("AdminSocket: terminated: {}", ep);
+          }
+        });
       });
-  });
+    }).then([] {
+      logger().debug("AdminSocket::init(): admin-sock thread terminated");
+      return seastar::now();
+    });
 
   return seastar::make_ready_future<>();
 }
 
 seastar::future<> AdminSocket::stop()
 {
-  if (m_server_sock && !arrivals_gate.is_closed()) {
-    // note that we check 'is_closed()' as if already closed - the server-sock
-    // may have already been discarded
-    m_server_sock->abort_accept();
-    m_server_sock = nullptr;
+  if (!server_sock) {
+    return seastar::now();
   }
-
-  return seastar::futurize_apply([this] { return arrivals_gate.close(); })
-    .then_wrapped([](seastar::future<> res) {
-      if (res.failed()) {
-        std::ignore = res.handle_exception([](std::exception_ptr eptr) {
-          return seastar::make_ready_future<>();
-        });
-      }
-      return seastar::make_ready_future<>();
-    }).handle_exception(
-      [](std::exception_ptr eptr) { return seastar::make_ready_future<>();
-    }).finally([] { return seastar::make_ready_future<>(); });
+  server_sock->abort_accept();
+  return stop_gate.close();
 }
 
 /////////////////////////////////////////

diff --git a/src/crimson/admin/admin_socket.h b/src/crimson/admin/admin_socket.h
index f8c74d0b8df9a67f6f9f40c23b84315657c333be..1b049aaeaf210800674754f29d3cf3dff71c8379 100644 (file)
--- a/src/crimson/admin/admin_socket.h
+++ b/src/crimson/admin/admin_socket.h
@@ -137,16 +137,12 @@ class AdminSocket : public seastar::enable_lw_shared_from_this<AdminSocket> {
                         const std::string& command_text,
                         ceph::bufferlist& out) const;
 
-  /**
-   * Non-owning ptr to the UNIX-domain "server-socket".
-   * Named here to allow a call to abort_accept().
-   */
-  seastar::api_v2::server_socket* m_server_sock{ nullptr };
+  std::optional<seastar::server_socket> server_sock;
 
   /**
    * stopping incoming ASOK requests at shutdown
    */
-  seastar::gate arrivals_gate;
+  seastar::gate stop_gate;
 
   /**
    *  parse the incoming command line into the sequence of words that identifies