[PATCH 0/4] Outdate disconnected session

[PATCH 0/4] Outdate disconnected session

[Vladislav Shpilevoy]

Once a connection is closed, a long-running user request can not
learn about this occasion. Even box.sesion.push() still works and
on_disconnect triggers are not run.

This patch makes things simpler:

* disconnected session is marked as dead. So a user can determine
  if a connection is closed by looking at session type == 'dead',
  or checking for errors from box.session.push();

* on_disconnect triggers are run right after disconnect. Even if
  there are some active requests.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-3859-outdate-session-on-disconnect
Issue: https://github.com/tarantool/tarantool/issues/3859

Vladislav Shpilevoy (4):
  iproto: rename disconnect cmsg to destroy
  iproto: fire on_disconnect right after disconnect
  session: introduce 'dead' type
  session: kill a session of a closed connection

 src/box/iproto.cc         | 78 +++++++++++++++++++++++++++------------
 src/box/session.cc        |  2 +
 src/box/session.h         | 14 +++++++
 test/box/net.box.result   | 21 ++++++++---
 test/box/net.box.test.lua | 13 +++++--
 test/box/push.result      | 50 +++++++++++++++++++++++++
 test/box/push.test.lua    | 22 +++++++++++
 7 files changed, 169 insertions(+), 31 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH 1/4] iproto: rename disconnect cmsg to destroy

[Vladislav Shpilevoy]

Disconnect cmsg is a message that is used by iproto
thread to notify tx thread that a connection is dead
and has no outstanding requests. That is its
tx-related resourses are freed and the connection is
deleted.

But the text above is clear definition of destroy. The
patch harmonizes cmsg name and its puprose.

Secondly, the patch is motivated by #3859 which
requires separate disconnect and destroy phases.

Needed for #3859
---
 src/box/iproto.cc | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 3d13bad2f..40d456374 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -271,14 +271,14 @@ enum rmean_net_name {
 const char *rmean_net_strings[IPROTO_LAST] = { "SENT", "RECEIVED" };
 
 static void
-tx_process_disconnect(struct cmsg *m);
+tx_process_destroy(struct cmsg *m);
 
 static void
-net_finish_disconnect(struct cmsg *m);
+net_finish_destroy(struct cmsg *m);
 
-static const struct cmsg_hop disconnect_route[] = {
-	{ tx_process_disconnect, &net_pipe },
-	{ net_finish_disconnect, NULL },
+static const struct cmsg_hop destroy_route[] = {
+	{ tx_process_destroy, &net_pipe },
+	{ net_finish_destroy, NULL },
 };
 
 /**
@@ -397,10 +397,10 @@ struct iproto_connection
 	/** Logical session. */
 	struct session *session;
 	ev_loop *loop;
-	/* Pre-allocated disconnect msg. */
-	struct cmsg disconnect;
-	/** True if disconnect message is sent. Debug-only. */
-	bool is_disconnected;
+	/** Pre-allocated destroy msg. */
+	struct cmsg destroy_msg;
+	/** True if destroy message is sent. Debug-only. */
+	bool is_destroy_sent;
 	struct rlist in_stop_list;
 	/**
 	 * Kharon is used to implement box.session.push().
@@ -576,9 +576,9 @@ iproto_connection_close(struct iproto_connection *con)
 	 * twice.
 	 */
 	if (iproto_connection_is_idle(con)) {
-		assert(con->is_disconnected == false);
-		con->is_disconnected = true;
-		cpipe_push(&tx_pipe, &con->disconnect);
+		assert(! con->is_destroy_sent);
+		con->is_destroy_sent = true;
+		cpipe_push(&tx_pipe, &con->destroy_msg);
 	}
 	rlist_del(&con->in_stop_list);
 }
@@ -992,8 +992,8 @@ iproto_connection_new(int fd)
 	con->session = NULL;
 	rlist_create(&con->in_stop_list);
 	/* It may be very awkward to allocate at close. */
-	cmsg_init(&con->disconnect, disconnect_route);
-	con->is_disconnected = false;
+	cmsg_init(&con->destroy_msg, destroy_route);
+	con->is_destroy_sent = false;
 	con->tx.is_push_pending = false;
 	con->tx.is_push_sent = false;
 	return con;
@@ -1204,10 +1204,10 @@ tx_fiber_init(struct session *session, uint64_t sync)
  * as well as output buffers of the connection.
  */
 static void
-tx_process_disconnect(struct cmsg *m)
+tx_process_destroy(struct cmsg *m)
 {
 	struct iproto_connection *con =
-		container_of(m, struct iproto_connection, disconnect);
+		container_of(m, struct iproto_connection, destroy_msg);
 	if (con->session) {
 		tx_fiber_init(con->session, 0);
 		if (! rlist_empty(&session_on_disconnect))
@@ -1228,10 +1228,10 @@ tx_process_disconnect(struct cmsg *m)
  * and close the connection.
  */
 static void
-net_finish_disconnect(struct cmsg *m)
+net_finish_destroy(struct cmsg *m)
 {
 	struct iproto_connection *con =
-		container_of(m, struct iproto_connection, disconnect);
+		container_of(m, struct iproto_connection, destroy_msg);
 	/* Runs the trigger, which may yield. */
 	iproto_connection_delete(con);
 }
-- 
2.17.2 (Apple Git-113)

Re: [tarantool-patches] [PATCH 1/4] iproto: rename disconnect cmsg to destroy

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> Disconnect cmsg is a message that is used by iproto
> thread to notify tx thread that a connection is dead
> and has no outstanding requests. That is its
> tx-related resourses are freed and the connection is
> deleted.
> 
> But the text above is clear definition of destroy. The
> patch harmonizes cmsg name and its puprose.
> 
> Secondly, the patch is motivated by #3859 which
> requires separate disconnect and destroy phases.
> 

OK to push.

> Needed for #3859

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 1/4] iproto: rename disconnect cmsg to destroy

[Vladislav Shpilevoy]

On 07/12/2018 20:36, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>> Disconnect cmsg is a message that is used by iproto
>> thread to notify tx thread that a connection is dead
>> and has no outstanding requests. That is its
>> tx-related resourses are freed and the connection is
>> deleted.
>>
>> But the text above is clear definition of destroy. The
>> patch harmonizes cmsg name and its puprose.
>>
>> Secondly, the patch is motivated by #3859 which
>> requires separate disconnect and destroy phases.
>>
> 
> OK to push.
> 
>> Needed for #3859
> 

Pushed to 2.1 and 1.10.

[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)

Re: [tarantool-patches] [PATCH 2/4] iproto: fire on_disconnect right after disconnect

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> 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.

OK to push.

> Needed for #3859
 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 2/4] iproto: fire on_disconnect right after disconnect

[Vladislav Shpilevoy]

On 07/12/2018 20:37, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>> 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.
> 
> OK to push.
> 
>> Needed for #3859
>   
> 

Pushed to 2.1 and 1.10.

[PATCH 3/4] session: introduce 'dead' type

[Vladislav Shpilevoy]

If an iproto connection is closed, there are no way
how to determine if this happened. Except setting an
on_disconnect trigger which sets a global flag or
something.

To deal with such orphan requests a new session type
is introduced that can be checked inside a request.

Needed for #3859
---
 src/box/session.cc |  2 ++
 src/box/session.h  | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/box/session.cc b/src/box/session.cc
index f868e9ecc..b8e20609b 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -42,6 +42,7 @@ const char *session_type_strs[] = {
 	"console",
 	"repl",
 	"applier",
+	"dead",
 	"unknown",
 };
 
@@ -57,6 +58,7 @@ struct session_vtab session_vtab_registry[] = {
 	/* CONSOLE */ generic_session_vtab,
 	/* REPL */ generic_session_vtab,
 	/* APPLIER */ generic_session_vtab,
+	/* DEAD */ generic_session_vtab,
 };
 
 static struct mh_i64ptr_t *session_registry;
diff --git a/src/box/session.h b/src/box/session.h
index 6ed97dc46..d998d06a0 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -56,6 +56,7 @@ enum session_type {
 	SESSION_TYPE_CONSOLE,
 	SESSION_TYPE_REPL,
 	SESSION_TYPE_APPLIER,
+	SESSION_TYPE_DEAD,
 	session_type_MAX,
 };
 
@@ -306,6 +307,19 @@ session_sync(struct session *session)
 	return session_vtab_registry[session->type].sync(session);
 }
 
+/**
+ * Make a session invalid. Its type is changed to 'dead' that
+ * automatically changes its vtab to a generic implementation,
+ * returning errors on everything. Mostly it is used to be able
+ * to determine if an iproto connection is closed inside a running
+ * request.
+ */
+static inline void
+session_kill(struct session *session)
+{
+	session->type = SESSION_TYPE_DEAD;
+}
+
 /**
  * In a common case, a session does not support push. This
  * function always returns -1 and sets ER_UNSUPPORTED error.
-- 
2.17.2 (Apple Git-113)

Re: [tarantool-patches] [PATCH 3/4] session: introduce 'dead' type

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> If an iproto connection is closed, there are no way
> how to determine if this happened. Except setting an
> on_disconnect trigger which sets a global flag or
> something.
> 
> To deal with such orphan requests a new session type
> is introduced that can be checked inside a request.

Please don't reset session type when it becomes dead.
What's the problem with storing an explicit vtab pointer in the
session and resetting it? 
> 
> Needed for #3859
> ---
>  src/box/session.cc |  2 ++
>  src/box/session.h  | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 3/4] session: introduce 'dead' type

[Vladislav Shpilevoy]

On 07/12/2018 20:38, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>> If an iproto connection is closed, there are no way
>> how to determine if this happened. Except setting an
>> on_disconnect trigger which sets a global flag or
>> something.
>>
>> To deal with such orphan requests a new session type
>> is introduced that can be checked inside a request.
> 
> Please don't reset session type when it becomes dead.
> What's the problem with storing an explicit vtab pointer in the
> session and resetting it?

I've explained to you what is a problem. To store vtab in struct
session we should reset its type never, since vtab now depends
both on type and on session state. But we can not do it because of
repl and console session types which are reset in src/lua/init.c and
src/lua/console.lua. Moreover, we can not set a session type repl
at start of the lua script runner, since it is in src/ but session
is in box/.

Also, I can turn background session type into console one via
require('console').start(), and this case it impossible to
determine at start of a fiber.

So the simplest ways to solve #3859 are 1) introduce 'dead' session
as we discussed with Vova, 2) just add an explicit check into
iproto session push implementation that a session is still connected.

Also we can move struct session into src/ but it is a long and complex
way that should be discussed.

>>
>> Needed for #3859
>> ---
>>   src/box/session.cc |  2 ++
>>   src/box/session.h  | 14 ++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
> 

Re: [tarantool-patches] Re: [PATCH 3/4] session: introduce 'dead' type

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/08 01:20]:
> On 07/12/2018 20:38, Konstantin Osipov wrote:
> > * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> > > If an iproto connection is closed, there are no way
> > > how to determine if this happened. Except setting an
> > > on_disconnect trigger which sets a global flag or
> > > something.
> > > 
> > > To deal with such orphan requests a new session type
> > > is introduced that can be checked inside a request.
> > 
> > Please don't reset session type when it becomes dead.
> > What's the problem with storing an explicit vtab pointer in the
> > session and resetting it?
> 
> I've explained to you what is a problem. To store vtab in struct
> session we should reset its type never, since vtab now depends
> both on type and on session state. But we can not do it because of
> repl and console session types which are reset in src/lua/init.c and
> src/lua/console.lua. Moreover, we can not set a session type repl
> at start of the lua script runner, since it is in src/ but session
> is in box/.

What's wrong with adding a method session_set_type() which would
reset both type and vtab?

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 3/4] session: introduce 'dead' type

[Vladislav Shpilevoy]

On 08/12/2018 01:21, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/08 01:20]:
>> On 07/12/2018 20:38, Konstantin Osipov wrote:
>>> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>>>> If an iproto connection is closed, there are no way
>>>> how to determine if this happened. Except setting an
>>>> on_disconnect trigger which sets a global flag or
>>>> something.
>>>>
>>>> To deal with such orphan requests a new session type
>>>> is introduced that can be checked inside a request.
>>>
>>> Please don't reset session type when it becomes dead.
>>> What's the problem with storing an explicit vtab pointer in the
>>> session and resetting it?
>>
>> I've explained to you what is a problem. To store vtab in struct
>> session we should reset its type never, since vtab now depends
>> both on type and on session state. But we can not do it because of
>> repl and console session types which are reset in src/lua/init.c and
>> src/lua/console.lua. Moreover, we can not set a session type repl
>> at start of the lua script runner, since it is in src/ but session
>> is in box/.
> 
> What's wrong with adding a method session_set_type() which would
> reset both type and vtab?
> 

I will try to implement it. But as I remember you were against it
yesterday:

Константин Осипов, [6 дек. 2018 г., 23:24:43]:
мне кажется проблема в другом

в том что я разрешаю ресетить type

эту проблему можно порешать, да

я согласен что переустановка типа сессии после её создания ломает инкапсуляцию.

Vladislav Shpilevoy, [6 дек. 2018 г., 23:25:23]:
я ее порешал, удалив repl. Теперь lbox_session_create всегда создает консоль

Константин Осипов, [6 дек. 2018 г., 23:25:23]:
это говнокод какой-то

Vladislav Shpilevoy, [6 дек. 2018 г., 23:26:03]:
если не удалять репл, то придется херачить что-то вроде session_set_type, причем надо будет запрещать конвертации несовместимых сессий

Константин Осипов, [6 дек. 2018 г., 23:26:12]:
воу воу

нахер вообще нужен lbox_session_create?

Nonetheless, as you wish.

[PATCH 4/4] session: kill a session of a closed connection

[Vladislav Shpilevoy]

Once a connection is closed, a long-running user
request can not learn about this occasion. Even
box.sesion.push() still works.

This patch makes such disconnected session dead. So
a user can determine if a connection is closed by
looking at session type == 'dead', or checking for
errors from box.session.push().

Closes #3859
---
 src/box/iproto.cc      |  9 +++++---
 test/box/push.result   | 50 ++++++++++++++++++++++++++++++++++++++++++
 test/box/push.test.lua | 22 +++++++++++++++++++
 3 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 19ee67958..b6c2ab587 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1225,9 +1225,12 @@ 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);
+	if (con->session != NULL) {
+		session_kill(con->session);
+		if (! rlist_empty(&session_on_disconnect)) {
+			tx_fiber_init(con->session, 0);
+			session_run_on_disconnect_triggers(con->session);
+		}
 	}
 }
 
diff --git a/test/box/push.result b/test/box/push.result
index af730c1a7..d2f6a6803 100644
--- a/test/box/push.result
+++ b/test/box/push.result
@@ -497,6 +497,56 @@ box.schema.func.drop('do_pushes')
 s:drop()
 ---
 ...
+--
+-- gh-3859: box.session.push() succeeds even after the connection
+-- is closed.
+--
+chan_push = fiber.channel()
+---
+...
+chan_disconnected = fiber.channel()
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function do_long_and_push()
+    box.session.on_disconnect(function() chan_disconnected:put(true) end)
+    chan_push:get()
+    ok, err = box.session.push(100)
+    chan_push:put(err)
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+box.schema.func.create('do_long_and_push')
+---
+...
+box.schema.user.grant('guest', 'execute', 'function', 'do_long_and_push')
+---
+...
+f = fiber.create(function() c:call('do_long_and_push') end)
+---
+...
 c:close()
 ---
 ...
+chan_disconnected:get()
+---
+- true
+...
+chan_push:put(true)
+---
+- true
+...
+chan_push:get()
+---
+- Session 'dead' does not support push()
+...
+box.schema.func.drop('do_long_and_push')
+---
+...
diff --git a/test/box/push.test.lua b/test/box/push.test.lua
index 893cf0153..514c08b3e 100644
--- a/test/box/push.test.lua
+++ b/test/box/push.test.lua
@@ -237,4 +237,26 @@ keys
 box.schema.func.drop('do_push_and_duplicate')
 box.schema.func.drop('do_pushes')
 s:drop()
+
+--
+-- gh-3859: box.session.push() succeeds even after the connection
+-- is closed.
+--
+chan_push = fiber.channel()
+chan_disconnected = fiber.channel()
+test_run:cmd("setopt delimiter ';'")
+function do_long_and_push()
+    box.session.on_disconnect(function() chan_disconnected:put(true) end)
+    chan_push:get()
+    ok, err = box.session.push(100)
+    chan_push:put(err)
+end;
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('do_long_and_push')
+box.schema.user.grant('guest', 'execute', 'function', 'do_long_and_push')
+f = fiber.create(function() c:call('do_long_and_push') end)
 c:close()
+chan_disconnected:get()
+chan_push:put(true)
+chan_push:get()
+box.schema.func.drop('do_long_and_push')
-- 
2.17.2 (Apple Git-113)

Re: [tarantool-patches] [PATCH 0/4] Outdate disconnected session

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> Once a connection is closed, a long-running user request can not
> learn about this occasion. Even box.sesion.push() still works and
> on_disconnect triggers are not run.
> 
> This patch makes things simpler:
> 
> * disconnected session is marked as dead. So a user can determine
>   if a connection is closed by looking at session type == 'dead',
>   or checking for errors from box.session.push();

'dead' is not a session type, it's session state. Please add a
separate method for this purpose, and not abuse the type.
> 
> * on_disconnect triggers are run right after disconnect. Even if
>   there are some active requests.

This is an incompatible change. But people have been actually
complaining in the chat that disconnect triggers are only run
after all messages are processed. So a good one. 


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 0/4] Outdate disconnected session

[Vladislav Shpilevoy]

On 07/12/2018 20:35, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>> Once a connection is closed, a long-running user request can not
>> learn about this occasion. Even box.sesion.push() still works and
>> on_disconnect triggers are not run.
>>
>> This patch makes things simpler:
>>
>> * disconnected session is marked as dead. So a user can determine
>>    if a connection is closed by looking at session type == 'dead',
>>    or checking for errors from box.session.push();
> 
> 'dead' is not a session type, it's session state. Please add a
> separate method for this purpose, and not abuse the type.

Done. See v2.

>>
>> * on_disconnect triggers are run right after disconnect. Even if
>>    there are some active requests.
> 
> This is an incompatible change. But people have been actually
> complaining in the chat that disconnect triggers are only run
> after all messages are processed. So a good one.
> 
> 

Re: [tarantool-patches] [PATCH 0/4] Outdate disconnected session

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
> Once a connection is closed, a long-running user request can not
> learn about this occasion. Even box.sesion.push() still works and
> on_disconnect triggers are not run.
> 
> This patch makes things simpler:
> 
> * disconnected session is marked as dead. So a user can determine
>   if a connection is closed by looking at session type == 'dead',
>   or checking for errors from box.session.push();
> 
> * on_disconnect triggers are run right after disconnect. Even if
>   there are some active requests.

I missed two comments: 

- please a docbot request to one of your changeset comments
- please push both into 2.1 and 1.10, use cherry-picking according to
  the new rules.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH 0/4] Outdate disconnected session

[Vladislav Shpilevoy]

On 07/12/2018 20:41, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/07 18:48]:
>> Once a connection is closed, a long-running user request can not
>> learn about this occasion. Even box.sesion.push() still works and
>> on_disconnect triggers are not run.
>>
>> This patch makes things simpler:
>>
>> * disconnected session is marked as dead. So a user can determine
>>    if a connection is closed by looking at session type == 'dead',
>>    or checking for errors from box.session.push();
>>
>> * on_disconnect triggers are run right after disconnect. Even if
>>    there are some active requests.
> 
> I missed two comments:
> 
> - please a docbot request to one of your changeset comments
> - please push both into 2.1 and 1.10, use cherry-picking according to
>    the new rules.
> 

Doc: https://github.com/tarantool/doc/issues/682
First two commits are cherry-picked to 2.1 and 1.10.