[PATCH v2 0/4] Outdate binary session on disconnect

[PATCH v2 0/4] Outdate binary session on disconnect

[Vladislav Shpilevoy]

Once a connection is closed, a user can learn it using
on_disconnect triggers. But session methods still work:
session.fd() returns socket fd, session.sync() returns request
sync, session.push() realy writes data to obuf and sends it to
iproto thread.

The goal of this patchset is to help user detect session
disconnect even without setting on_disconnect triggers. Patchset
makes a session, whose socket is closed, rotten. It returns an
error on push, -1 on fd, 0 on sync.

This is reached via moving struct session_vtab from a global
array to struct session and changing the vtab on disconnect.

V1: https://www.freelists.org/post/tarantool-patches/PATCH-04-Outdate-disconnected-session

Changes in v2:
- Store vtab in struct session, remove session_vtab_registry;
- Do not change session type on disconnect. Only vtab.

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):
  console: forbid arbitrary session type setting
  session: minimize number of session type resets
  session: store vtab in struct session
  session: outdate a session of a closed connection

 src/box/applier.cc      |  6 +++-
 src/box/iproto.cc       | 77 +++++++++++++++++++++++++++++------------
 src/box/lua/console.c   | 15 ++++----
 src/box/lua/console.lua |  4 +--
 src/box/lua/session.c   | 26 ++++++++++----
 src/box/session.cc      | 19 ++++------
 src/box/session.h       | 29 +++++++++++-----
 test/box/push.result    | 50 ++++++++++++++++++++++++++
 test/box/push.test.lua  | 22 ++++++++++++
 9 files changed, 186 insertions(+), 62 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH v2 1/4] console: forbid arbitrary session type setting

[Vladislav Shpilevoy]

The only place where session type is changed after its
creation is console. Console sets 'repl' session type
once console.start() is called, or 'console', if a
remote client connected to console server using
console.connect().

But function to set session type is too common. Using
it any other session type can be set. Even console
can be changed to applier or binary. Lets split it
into two more specific functions setting only 'repl'
and 'console'.

Also, it simplifies next patches, making vtab part of
struct session.
---
 src/box/lua/console.lua |  4 ++--
 src/box/lua/session.c   | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 028001127..19ea4bb0a 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -448,7 +448,7 @@ local function start()
         self.history_file = home_dir .. '/.tarantool_history'
         internal.load_history(self.history_file)
     end
-    session_internal.create(1, "repl") -- stdin fileno
+    session_internal.create_repl(1) -- stdin fileno
     repl(self)
     started = false
 end
@@ -521,7 +521,7 @@ local function connect(uri, opts)
 end
 
 local function client_handler(client, peer)
-    session_internal.create(client:fd(), "console")
+    session_internal.create_console(client:fd())
     session_internal.run_on_connect()
     session_internal.run_on_auth(box.session.user(), true)
     local state = setmetatable({
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index d3d27643f..f4944baea 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -47,22 +47,33 @@ static const char *sessionlib_name = "box.session";
 
 /* Create session and pin it to fiber */
 static int
-lbox_session_create(struct lua_State *L)
+lbox_session_create(struct lua_State *L, enum session_type type)
 {
 	struct session *session = fiber_get_session(fiber());
 	if (session == NULL) {
 		session = session_create_on_demand();
 		if (session == NULL)
 			return luaT_error(L);
-		session->meta.fd = luaL_optinteger(L, 1, -1);
+		session->meta.fd = lua_tointeger(L, 1);
 	}
-	/* If a session already exists, simply reset its type */
-	session->type = STR2ENUM(session_type, luaL_optstring(L, 2, "console"));
+	session->type = type;
 
 	lua_pushnumber(L, session->id);
 	return 1;
 }
 
+static int
+lbox_session_create_console(struct lua_State *L)
+{
+	return lbox_session_create(L, SESSION_TYPE_CONSOLE);
+}
+
+static int
+lbox_session_create_repl(struct lua_State *L)
+{
+	return lbox_session_create(L, SESSION_TYPE_REPL);
+}
+
 /**
  * Return a unique monotonic session
  * identifier. The identifier can be used
@@ -446,7 +457,8 @@ void
 box_lua_session_init(struct lua_State *L)
 {
 	static const struct luaL_Reg session_internal_lib[] = {
-		{"create", lbox_session_create},
+		{"create_console", lbox_session_create_console},
+		{"create_repl", lbox_session_create_repl},
 		{"run_on_connect",    lbox_session_run_on_connect},
 		{"run_on_disconnect", lbox_session_run_on_disconnect},
 		{"run_on_auth", lbox_session_run_on_auth},
-- 
2.17.2 (Apple Git-113)

[PATCH v2 2/4] session: minimize number of session type resets

[Vladislav Shpilevoy]

Strictly speaking, session type change after its
creation is illegal. But. There are one case when it
is ok and inevitable: 'console' and 'repl' sessions.
'Repl' session is just an interactive console from
stdin run as console.start(). 'Console' is a session,
created on console.connect().

'Repl' can not be set right after fiber creation
because struct session depends on box/, however
src/lua/init.c, which starts 'repl', does not depend
on it.

'Console' can not be set, because a user can call
console.start() at any moment of his lua script.

This patch removes other unnecessary session type
changes and wraps 'console'/'repl' with
session_set_type().
---
 src/box/applier.cc    |  5 ++++-
 src/box/lua/session.c |  5 +++--
 src/box/session.cc    |  6 +++---
 src/box/session.h     | 12 +++++++++---
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index ff4af95e5..4b5b851e6 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -587,7 +587,10 @@ applier_f(va_list ap)
 	 * Set correct session type for use in on_replace()
 	 * triggers.
 	 */
-	current_session()->type = SESSION_TYPE_APPLIER;
+	if (session_create_on_demand(SESSION_TYPE_APPLIER) == NULL) {
+		diag_log();
+		return -1;
+	}
 
 	/* Re-connect loop */
 	while (!fiber_is_cancelled()) {
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index f4944baea..51582f749 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -51,12 +51,13 @@ lbox_session_create(struct lua_State *L, enum session_type type)
 {
 	struct session *session = fiber_get_session(fiber());
 	if (session == NULL) {
-		session = session_create_on_demand();
+		session = session_create_on_demand(type);
 		if (session == NULL)
 			return luaT_error(L);
 		session->meta.fd = lua_tointeger(L, 1);
+	} else {
+		session_set_type(session, type);
 	}
-	session->type = type;
 
 	lua_pushnumber(L, session->id);
 	return 1;
diff --git a/src/box/session.cc b/src/box/session.cc
index 64714cdcf..36da9ce69 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -105,7 +105,7 @@ session_create(enum session_type type)
 
 	session->id = sid_max();
 	memset(&session->meta, 0, sizeof(session->meta));
-	session->type = type;
+	session_set_type(session, type);
 	session->sql_flags = default_flags;
 	session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX;
 
@@ -127,12 +127,12 @@ session_create(enum session_type type)
 }
 
 struct session *
-session_create_on_demand()
+session_create_on_demand(enum session_type type)
 {
 	assert(fiber_get_session(fiber()) == NULL);
 
 	/* Create session on demand */
-	struct session *s = session_create(SESSION_TYPE_BACKGROUND);
+	struct session *s = session_create(type);
 	if (s == NULL)
 		return NULL;
 	s->fiber_on_stop = {
diff --git a/src/box/session.h b/src/box/session.h
index df1dcbc62..45f0899b1 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -140,6 +140,12 @@ struct session_vtab {
 
 extern struct session_vtab session_vtab_registry[];
 
+static inline void
+session_set_type(struct session *session, enum session_type new_type)
+{
+	session->type = new_type;
+}
+
 /**
  * Find a session by id.
  */
@@ -200,7 +206,7 @@ extern struct credentials admin_credentials;
  * trigger to destroy it when this fiber ends.
  */
 struct session *
-session_create_on_demand();
+session_create_on_demand(enum session_type type);
 
 /*
  * When creating a new fiber, the database (box)
@@ -217,7 +223,7 @@ current_session()
 {
 	struct session *session = fiber_get_session(fiber());
 	if (session == NULL) {
-		session = session_create_on_demand();
+		session = session_create_on_demand(SESSION_TYPE_BACKGROUND);
 		if (session == NULL)
 			diag_raise();
 	}
@@ -236,7 +242,7 @@ effective_user()
 	struct fiber *f = fiber();
 	struct credentials *u = f->storage.credentials;
 	if (u == NULL) {
-		session_create_on_demand();
+		session_create_on_demand(SESSION_TYPE_BACKGROUND);
 		u = f->storage.credentials;
 	}
 	return u;
-- 
2.17.2 (Apple Git-113)

[PATCH v2 3/4] session: store vtab in struct session

[Vladislav Shpilevoy]

Before the patch vtabs were stored in a global array
accessed by session.type when necessary. But it does
not allow to change session vtab without affecting
session type. This feature is necessary to be able to
outdate binary sessions whose socket is closed.

Outdated session will return errors from all its
methods.

Needed for #3859
---
 src/box/applier.cc    |  3 ++-
 src/box/iproto.cc     | 50 ++++++++++++++++++++++++++-----------------
 src/box/lua/console.c | 15 ++++++-------
 src/box/lua/session.c |  5 +++--
 src/box/session.cc    | 19 ++++++----------
 src/box/session.h     | 25 ++++++++++++++--------
 6 files changed, 63 insertions(+), 54 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index 4b5b851e6..1d9327d42 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -587,7 +587,8 @@ applier_f(va_list ap)
 	 * Set correct session type for use in on_replace()
 	 * triggers.
 	 */
-	if (session_create_on_demand(SESSION_TYPE_APPLIER) == NULL) {
+	if (session_create_on_demand(SESSION_TYPE_APPLIER,
+				     &generic_session_vtab) == NULL) {
 		diag_log();
 		return -1;
 	}
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 03066dd59..e6eefc3d8 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1747,6 +1747,32 @@ net_end_subscribe(struct cmsg *m)
 	iproto_connection_close(con);
 }
 
+static int
+iproto_session_fd(struct session *session);
+
+static int64_t
+iproto_session_sync(struct session *session);
+
+/**
+ * Push a message from @a port to a remote client.
+ * @param session iproto session.
+ * @param sync Request sync in scope of which to send the push.
+ * @param port Port with data to send.
+ *
+ * @retval -1 Memory error.
+ * @retval  0 Success, a message is written to the output buffer.
+ *            We don't wait here that the push has reached the
+ *            client: the output buffer is flushed asynchronously.
+ */
+static int
+iproto_session_push(struct session *session, uint64_t sync, struct port *port);
+
+static const struct session_vtab iproto_session_vtab = {
+	/* .push = */ iproto_session_push,
+	/* .fd = */ iproto_session_fd,
+	/* .sync = */ iproto_session_sync,
+};
+
 /**
  * Handshake a connection: invoke the on-connect trigger
  * and possibly authenticate. Try to send the client an error
@@ -1759,7 +1785,8 @@ tx_process_connect(struct cmsg *m)
 	struct iproto_connection *con = msg->connection;
 	struct obuf *out = msg->connection->tx.p_obuf;
 	try {              /* connect. */
-		con->session = session_create(SESSION_TYPE_BINARY);
+		con->session = session_create(SESSION_TYPE_BINARY,
+					      &iproto_session_vtab);
 		if (con->session == NULL)
 			diag_raise();
 		con->session->meta.connection = con;
@@ -1905,7 +1932,7 @@ net_cord_f(va_list /* ap */)
 	return 0;
 }
 
-int
+static int
 iproto_session_fd(struct session *session)
 {
 	struct iproto_connection *con =
@@ -1913,7 +1940,7 @@ iproto_session_fd(struct session *session)
 	return con->output.fd;
 }
 
-int64_t
+static int64_t
 iproto_session_sync(struct session *session)
 {
 	(void) session;
@@ -1962,17 +1989,6 @@ tx_end_push(struct cmsg *m)
 		tx_begin_push(con);
 }
 
-/**
- * Push a message from @a port to a remote client.
- * @param session iproto session.
- * @param sync Request sync in scope of which to send the push.
- * @param port Port with data to send.
- *
- * @retval -1 Memory error.
- * @retval  0 Success, a message is written to the output buffer.
- *            We don't wait here that the push has reached the
- *            client: the output buffer is flushed asynchronously.
- */
 static int
 iproto_session_push(struct session *session, uint64_t sync, struct port *port)
 {
@@ -2007,12 +2023,6 @@ iproto_init()
 	/* Create a pipe to "net" thread. */
 	cpipe_create(&net_pipe, "net");
 	cpipe_set_max_input(&net_pipe, iproto_msg_max / 2);
-	struct session_vtab iproto_session_vtab = {
-		/* .push = */ iproto_session_push,
-		/* .fd = */ iproto_session_fd,
-		/* .sync = */ iproto_session_sync,
-	};
-	session_vtab_registry[SESSION_TYPE_BINARY] = iproto_session_vtab;
 }
 
 /** Available iproto configuration changes. */
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index 085aedfaf..ef29ffc53 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -414,8 +414,6 @@ static int
 console_session_push(struct session *session, uint64_t sync, struct port *port)
 {
 	(void) sync;
-	assert(session_vtab_registry[session->type].push ==
-	       console_session_push);
 	uint32_t text_len;
 	const char *text = port_dump_plain(port, &text_len);
 	if (text == NULL)
@@ -424,6 +422,12 @@ console_session_push(struct session *session, uint64_t sync, struct port *port)
 				     TIMEOUT_INFINITY);
 }
 
+const struct session_vtab console_session_vtab = {
+	/* .push = */ console_session_push,
+	/* .fd = */ console_session_fd,
+	/* .sync = */ generic_session_sync,
+};
+
 void
 tarantool_lua_console_init(struct lua_State *L)
 {
@@ -457,13 +461,6 @@ tarantool_lua_console_init(struct lua_State *L)
 	 * load_history work the same way.
 	 */
 	lua_setfield(L, -2, "formatter");
-	struct session_vtab console_session_vtab = {
-		/* .push = */ console_session_push,
-		/* .fd = */ console_session_fd,
-		/* .sync = */ generic_session_sync,
-	};
-	session_vtab_registry[SESSION_TYPE_CONSOLE] = console_session_vtab;
-	session_vtab_registry[SESSION_TYPE_REPL] = console_session_vtab;
 }
 
 /*
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 51582f749..ba45ef62b 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -44,6 +44,7 @@
 #include "box/port.h"
 
 static const char *sessionlib_name = "box.session";
+extern const struct session_vtab console_session_vtab;
 
 /* Create session and pin it to fiber */
 static int
@@ -51,12 +52,12 @@ lbox_session_create(struct lua_State *L, enum session_type type)
 {
 	struct session *session = fiber_get_session(fiber());
 	if (session == NULL) {
-		session = session_create_on_demand(type);
+		session = session_create_on_demand(type, &console_session_vtab);
 		if (session == NULL)
 			return luaT_error(L);
 		session->meta.fd = lua_tointeger(L, 1);
 	} else {
-		session_set_type(session, type);
+		session_set_type(session, type, &console_session_vtab);
 	}
 
 	lua_pushnumber(L, session->id);
diff --git a/src/box/session.cc b/src/box/session.cc
index 36da9ce69..216057921 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -45,20 +45,12 @@ const char *session_type_strs[] = {
 	"unknown",
 };
 
-static struct session_vtab generic_session_vtab = {
+const struct session_vtab generic_session_vtab = {
 	/* .push = */ generic_session_push,
 	/* .fd = */ generic_session_fd,
 	/* .sync = */ generic_session_sync,
 };
 
-struct session_vtab session_vtab_registry[] = {
-	/* BACKGROUND */ generic_session_vtab,
-	/* BINARY */ generic_session_vtab,
-	/* CONSOLE */ generic_session_vtab,
-	/* REPL */ generic_session_vtab,
-	/* APPLIER */ generic_session_vtab,
-};
-
 static struct mh_i64ptr_t *session_registry;
 
 uint32_t default_flags = 0;
@@ -93,7 +85,7 @@ session_on_stop(struct trigger *trigger, void * /* event */)
 }
 
 struct session *
-session_create(enum session_type type)
+session_create(enum session_type type, const struct session_vtab *vtab)
 {
 	struct session *session =
 		(struct session *) mempool_alloc(&session_pool);
@@ -105,7 +97,7 @@ session_create(enum session_type type)
 
 	session->id = sid_max();
 	memset(&session->meta, 0, sizeof(session->meta));
-	session_set_type(session, type);
+	session_set_type(session, type, vtab);
 	session->sql_flags = default_flags;
 	session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX;
 
@@ -127,12 +119,13 @@ session_create(enum session_type type)
 }
 
 struct session *
-session_create_on_demand(enum session_type type)
+session_create_on_demand(enum session_type type,
+			 const struct session_vtab *vtab)
 {
 	assert(fiber_get_session(fiber()) == NULL);
 
 	/* Create session on demand */
-	struct session *s = session_create(type);
+	struct session *s = session_create(type, vtab);
 	if (s == NULL)
 		return NULL;
 	s->fiber_on_stop = {
diff --git a/src/box/session.h b/src/box/session.h
index 45f0899b1..631d649e2 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -97,6 +97,8 @@ struct session {
 	/** SQL Connection flag for current user session */
 	uint32_t sql_flags;
 	enum session_type type;
+	/** Session virtual methods. */
+	const struct session_vtab *vtab;
 	/** Session metadata. */
 	union session_meta meta;
 	/** Session user id and global grants */
@@ -138,12 +140,14 @@ struct session_vtab {
 	(*sync)(struct session *session);
 };
 
-extern struct session_vtab session_vtab_registry[];
+extern const struct session_vtab generic_session_vtab;
 
 static inline void
-session_set_type(struct session *session, enum session_type new_type)
+session_set_type(struct session *session, enum session_type new_type,
+		 const struct session_vtab *vtab)
 {
 	session->type = new_type;
+	session->vtab = vtab;
 }
 
 /**
@@ -206,7 +210,8 @@ extern struct credentials admin_credentials;
  * trigger to destroy it when this fiber ends.
  */
 struct session *
-session_create_on_demand(enum session_type type);
+session_create_on_demand(enum session_type type,
+			 const struct session_vtab *vtab);
 
 /*
  * When creating a new fiber, the database (box)
@@ -223,7 +228,8 @@ current_session()
 {
 	struct session *session = fiber_get_session(fiber());
 	if (session == NULL) {
-		session = session_create_on_demand(SESSION_TYPE_BACKGROUND);
+		session = session_create_on_demand(SESSION_TYPE_BACKGROUND,
+						   &generic_session_vtab);
 		if (session == NULL)
 			diag_raise();
 	}
@@ -242,7 +248,8 @@ effective_user()
 	struct fiber *f = fiber();
 	struct credentials *u = f->storage.credentials;
 	if (u == NULL) {
-		session_create_on_demand(SESSION_TYPE_BACKGROUND);
+		session_create_on_demand(SESSION_TYPE_BACKGROUND,
+					 &generic_session_vtab);
 		u = f->storage.credentials;
 	}
 	return u;
@@ -266,7 +273,7 @@ session_storage_cleanup(int sid);
  * trigger fails or runs out of resources.
  */
 struct session *
-session_create(enum session_type type);
+session_create(enum session_type type, const struct session_vtab *vtab);
 
 /**
  * Destroy a session.
@@ -308,19 +315,19 @@ access_check_universe(user_access_t access);
 static inline int
 session_push(struct session *session, uint64_t sync, struct port *port)
 {
-	return session_vtab_registry[session->type].push(session, sync, port);
+	return session->vtab->push(session, sync, port);
 }
 
 static inline int
 session_fd(struct session *session)
 {
-	return session_vtab_registry[session->type].fd(session);
+	return session->vtab->fd(session);
 }
 
 static inline int
 session_sync(struct session *session)
 {
-	return session_vtab_registry[session->type].sync(session);
+	return session->vtab->sync(session);
 }
 
 /**
-- 
2.17.2 (Apple Git-113)

[PATCH v2 4/4] session: outdate 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 'rotten'.
So a user can determine if a connection is closed by
looking at session.fd() == -1, or checking for
errors from box.session.push().

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

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index e6eefc3d8..8fddc76ef 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1239,14 +1239,35 @@ tx_fiber_init(struct session *session, uint64_t sync)
 	fiber_set_user(f, &session->credentials);
 }
 
+static int
+iproto_disconnected_session_push(struct session *session, uint64_t sync,
+				 struct port *port)
+{
+	(void) session;
+	(void) sync;
+	(void) port;
+	diag_set(ClientError, ER_NO_CONNECTION);
+	return -1;
+}
+
+static const struct session_vtab iproto_disconnected_session_vtab = {
+	/* .push = */ iproto_disconnected_session_push,
+	/* .fd = */ generic_session_fd,
+	/* .sync = */ generic_session_sync,
+};
+
 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);
+	if (con->session != NULL) {
+		session_set_type(con->session, SESSION_TYPE_BINARY,
+				 &iproto_disconnected_session_vtab);
+		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..6f858f38a 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()
+---
+- Connection is not established
+...
+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: [PATCH v2 3/4] session: store vtab in struct session

[Vladimir Davydov]

On Tue, Dec 11, 2018 at 07:10:22PM +0300, Vladislav Shpilevoy wrote:
> @@ -587,7 +587,8 @@ applier_f(va_list ap)
>  	 * Set correct session type for use in on_replace()
>  	 * triggers.
>  	 */
> -	if (session_create_on_demand(SESSION_TYPE_APPLIER) == NULL) {
> +	if (session_create_on_demand(SESSION_TYPE_APPLIER,
> +				     &generic_session_vtab) == NULL) {
>  		diag_log();
>  		return -1;
>  	}

> @@ -51,12 +52,12 @@ lbox_session_create(struct lua_State *L, enum session_type type)
>  {
>  	struct session *session = fiber_get_session(fiber());
>  	if (session == NULL) {
> -		session = session_create_on_demand(type);
> +		session = session_create_on_demand(type, &console_session_vtab);
>  		if (session == NULL)
>  			return luaT_error(L);
>  		session->meta.fd = lua_tointeger(L, 1);
>  	} else {
> -		session_set_type(session, type);
> +		session_set_type(session, type, &console_session_vtab);
>  	}

IMHO the new API is confusing, because it allows to do e.g.

  session_set_type(SESSION_TYPE_APPLIER, &console_session_vtab);

What about leaving the session vtab registry and using it in
session_set_type to set session->vtab consistent with a given type?
Then, to kill a session we would introduce a separate method, which
would reset session->vtab. And we could add an assertion ensuring that
session_set_type isn't called on a dead session.

Re: [PATCH v2 1/4] console: forbid arbitrary session type setting

[Vladimir Davydov]

On Tue, Dec 11, 2018 at 07:10:20PM +0300, Vladislav Shpilevoy wrote:
> @@ -47,22 +47,33 @@ static const char *sessionlib_name = "box.session";
>  
>  /* Create session and pin it to fiber */
>  static int
> -lbox_session_create(struct lua_State *L)
> +lbox_session_create(struct lua_State *L, enum session_type type)
>  {
>  	struct session *session = fiber_get_session(fiber());
>  	if (session == NULL) {
>  		session = session_create_on_demand();
>  		if (session == NULL)
>  			return luaT_error(L);
> -		session->meta.fd = luaL_optinteger(L, 1, -1);
> +		session->meta.fd = lua_tointeger(L, 1);
>  	}
> -	/* If a session already exists, simply reset its type */
> -	session->type = STR2ENUM(session_type, luaL_optstring(L, 2, "console"));
> +	session->type = type;
>  
>  	lua_pushnumber(L, session->id);
>  	return 1;
>  }

If you didn't remove session_vtab registry, you wouldn't be needing this
patch. I don't think that narrowing functionality of an internal
function makes much sense on its own so please drop it if you agree to
leave the registry.

Re: [tarantool-patches] Re: [PATCH v2 3/4] session: store vtab in struct session

[Vladislav Shpilevoy]

On 21/12/2018 14:04, Vladimir Davydov wrote:
> On Tue, Dec 11, 2018 at 07:10:22PM +0300, Vladislav Shpilevoy wrote:
>> @@ -587,7 +587,8 @@ applier_f(va_list ap)
>>   	 * Set correct session type for use in on_replace()
>>   	 * triggers.
>>   	 */
>> -	if (session_create_on_demand(SESSION_TYPE_APPLIER) == NULL) {
>> +	if (session_create_on_demand(SESSION_TYPE_APPLIER,
>> +				     &generic_session_vtab) == NULL) {
>>   		diag_log();
>>   		return -1;
>>   	}
> 
>> @@ -51,12 +52,12 @@ lbox_session_create(struct lua_State *L, enum session_type type)
>>   {
>>   	struct session *session = fiber_get_session(fiber());
>>   	if (session == NULL) {
>> -		session = session_create_on_demand(type);
>> +		session = session_create_on_demand(type, &console_session_vtab);
>>   		if (session == NULL)
>>   			return luaT_error(L);
>>   		session->meta.fd = lua_tointeger(L, 1);
>>   	} else {
>> -		session_set_type(session, type);
>> +		session_set_type(session, type, &console_session_vtab);
>>   	}
> 
> IMHO the new API is confusing, because it allows to do e.g.
> 
>    session_set_type(SESSION_TYPE_APPLIER, &console_session_vtab);
> 
> What about leaving the session vtab registry and using it in
> session_set_type to set session->vtab consistent with a given type?

I do not like having both session->vtab and session_registry. Also,
Kostja asked to delete session_registry. But as you wish.

> Then, to kill a session we would introduce a separate method, which
> would reset session->vtab. And we could add an assertion ensuring that
> session_set_type isn't called on a dead session.
>