[PATCH 2/4] iproto: fire on_disconnect right after disconnect

[Vladislav Shpilevoy]

Before the patch on_disconnect triggers were called
only after last outstanding request had finished. It
was enough for most goals. But after box.session.push
was implemented, it appeared that it is impossible to
return an error from push() if a connection is closed.

Tx session just does not know that a connection is
already closed. The patch splits destroy and
disconnect phases of an iproto connection lifecycle.
Disconnect calls on_disconnect triggers and resets
iproto socket fd. Destroy frees resources.

Needed for #3859
---
 src/box/iproto.cc         | 43 ++++++++++++++++++++++++++++++++-------
 test/box/net.box.result   | 21 ++++++++++++++-----
 test/box/net.box.test.lua | 13 +++++++++---
 3 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 40d456374..19ee67958 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -281,6 +281,14 @@ static const struct cmsg_hop destroy_route[] = {
 	{ net_finish_destroy, NULL },
 };
 
+/** Fire on_disconnect triggers in the tx thread. */
+static void
+tx_process_disconnect(struct cmsg *m);
+
+static const struct cmsg_hop disconnect_route[] = {
+	{ tx_process_disconnect, NULL }
+};
+
 /**
  * Kharon is in the dead world (iproto). Schedule an event to
  * flush new obuf as reflected in the fresh wpos.
@@ -397,7 +405,19 @@ struct iproto_connection
 	/** Logical session. */
 	struct session *session;
 	ev_loop *loop;
-	/** Pre-allocated destroy msg. */
+	/**
+	 * Pre-allocated disconnect msg. Is sent right after
+	 * actual disconnect has happened. Does not destroy the
+	 * connection. Used to notify existing requests about the
+	 * occasion.
+	 */
+	struct cmsg disconnect_msg;
+	/**
+	 * Pre-allocated destroy msg. Is sent after disconnect has
+	 * happened and a last request has finished. Firstly
+	 * destroys tx-related resources and then deletes the
+	 * connection.
+	 */
 	struct cmsg destroy_msg;
 	/** True if destroy message is sent. Debug-only. */
 	bool is_destroy_sent;
@@ -562,6 +582,7 @@ iproto_connection_close(struct iproto_connection *con)
 		 * is done only once.
 		 */
 		con->p_ibuf->wpos -= con->parse_size;
+		cpipe_push(&tx_pipe, &con->disconnect_msg);
 	}
 	/*
 	 * If the connection has no outstanding requests in the
@@ -993,6 +1014,7 @@ iproto_connection_new(int fd)
 	rlist_create(&con->in_stop_list);
 	/* It may be very awkward to allocate at close. */
 	cmsg_init(&con->destroy_msg, destroy_route);
+	cmsg_init(&con->disconnect_msg, disconnect_route);
 	con->is_destroy_sent = false;
 	con->tx.is_push_pending = false;
 	con->tx.is_push_sent = false;
@@ -1198,10 +1220,20 @@ tx_fiber_init(struct session *session, uint64_t sync)
 	fiber_set_user(f, &session->credentials);
 }
 
+static void
+tx_process_disconnect(struct cmsg *m)
+{
+	struct iproto_connection *con =
+		container_of(m, struct iproto_connection, disconnect_msg);
+	if (con->session != NULL && !rlist_empty(&session_on_disconnect)) {
+		tx_fiber_init(con->session, 0);
+		session_run_on_disconnect_triggers(con->session);
+	}
+}
+
 /**
- * Fire on_disconnect triggers in the tx
- * thread and destroy the session object,
- * as well as output buffers of the connection.
+ * Destroy the session object, as well as output buffers of the
+ * connection.
  */
 static void
 tx_process_destroy(struct cmsg *m)
@@ -1209,9 +1241,6 @@ tx_process_destroy(struct cmsg *m)
 	struct iproto_connection *con =
 		container_of(m, struct iproto_connection, destroy_msg);
 	if (con->session) {
-		tx_fiber_init(con->session, 0);
-		if (! rlist_empty(&session_on_disconnect))
-			session_run_on_disconnect_triggers(con->session);
 		session_destroy(con->session);
 		con->session = NULL; /* safety */
 	}
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 6e59d0bc0..62534a545 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2261,6 +2261,11 @@ box.session.on_disconnect(on_disconnect) == on_disconnect
 ---
 - true
 ...
+--
+-- gh-3859: on_disconnect is called only after all requests are
+-- processed, but should be called right after disconnect and
+-- only once.
+--
 ch1 = fiber.channel(1)
 ---
 ...
@@ -2283,21 +2288,27 @@ c:close()
 fiber.sleep(0)
 ---
 ...
-disconnected -- false
+while disconnected == false do fiber.sleep(0.01) end
 ---
-- false
 ...
-ch2:put(true)
+disconnected -- true
 ---
 - true
 ...
-while disconnected == false do fiber.sleep(0.01) end
+disconnected = nil
 ---
 ...
-disconnected -- true
+ch2:put(true)
 ---
 - true
 ...
+fiber.sleep(0)
+---
+...
+disconnected -- nil, on_disconnect is not called second time.
+---
+- null
+...
 box.session.on_disconnect(nil, on_disconnect)
 ---
 ...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 26773aac9..d34d7affa 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -934,6 +934,11 @@ fiber.sleep(0)
 
 box.session.on_disconnect(on_disconnect) == on_disconnect
 
+--
+-- gh-3859: on_disconnect is called only after all requests are
+-- processed, but should be called right after disconnect and
+-- only once.
+--
 ch1 = fiber.channel(1)
 ch2 = fiber.channel(1)
 function wait_signal() ch1:put(true) ch2:get() end
@@ -942,11 +947,13 @@ ch1:get()
 
 c:close()
 fiber.sleep(0)
-disconnected -- false
-
-ch2:put(true)
 while disconnected == false do fiber.sleep(0.01) end
 disconnected -- true
+disconnected = nil
+
+ch2:put(true)
+fiber.sleep(0)
+disconnected -- nil, on_disconnect is not called second time.
 
 box.session.on_disconnect(nil, on_disconnect)
 
-- 
2.17.2 (Apple Git-113)