[PATCH v3 0/2] Outdate binary session on disconnect

[PATCH v3 0/2] 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
V2: https://www.freelists.org/post/tarantool-patches/PATCH-v2-04-Outdate-binary-session-on-disconnect

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

Changes in v3:
- Keep session_vtab_registry.

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

Vladislav Shpilevoy (2):
  session: store vtab both in struct and registry
  session: outdate a session of a closed connection

 src/box/applier.cc     |  5 ++++-
 src/box/errcode.h      |  2 +-
 src/box/iproto.cc      |  9 +++++---
 src/box/lua/session.c  |  3 ++-
 src/box/session.cc     | 33 +++++++++++++++++++++++++++-
 src/box/session.h      | 19 +++++++++++++---
 test/box/misc.result   |  1 +
 test/box/push.result   | 50 ++++++++++++++++++++++++++++++++++++++++++
 test/box/push.test.lua | 22 +++++++++++++++++++
 9 files changed, 134 insertions(+), 10 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH v3 1/2] session: store vtab both in struct and registry

[Vladislav Shpilevoy]

Before the patch vtabs were stored in a global array
only, called registry and accessed by session.type to
call a virtual method. 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, closed, sessions will return errors from all
its methods.

Needed for #3859
---
 src/box/applier.cc    |  5 ++++-
 src/box/lua/session.c |  3 ++-
 src/box/session.cc    | 10 +++++++++-
 src/box/session.h     | 12 +++++++++---
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index ff4af95e5..21d2e6bcb 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;
+	struct session *session = session_create_on_demand();
+	if (session == NULL)
+		return -1;
+	session_set_type(session, SESSION_TYPE_APPLIER);
 
 	/* Re-connect loop */
 	while (!fiber_is_cancelled()) {
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index d3d27643f..a4ddb201c 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -57,7 +57,8 @@ lbox_session_create(struct lua_State *L)
 		session->meta.fd = luaL_optinteger(L, 1, -1);
 	}
 	/* If a session already exists, simply reset its type */
-	session->type = STR2ENUM(session_type, luaL_optstring(L, 2, "console"));
+	session_set_type(session, STR2ENUM(session_type,
+					   luaL_optstring(L, 2, "console")));
 
 	lua_pushnumber(L, session->id);
 	return 1;
diff --git a/src/box/session.cc b/src/box/session.cc
index 64714cdcf..0ec118bca 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -92,6 +92,14 @@ session_on_stop(struct trigger *trigger, void * /* event */)
 	session_destroy(fiber_get_session(fiber()));
 }
 
+void
+session_set_type(struct session *session, enum session_type type)
+{
+	assert(type < session_type_MAX);
+	session->type = type;
+	session->vtab = &session_vtab_registry[type];
+}
+
 struct session *
 session_create(enum session_type type)
 {
@@ -105,7 +113,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;
 
diff --git a/src/box/session.h b/src/box/session.h
index df1dcbc62..d86939e48 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 */
@@ -140,6 +142,10 @@ struct session_vtab {
 
 extern struct session_vtab session_vtab_registry[];
 
+/** Change session type and vtab. */
+void
+session_set_type(struct session *session, enum session_type type);
+
 /**
  * Find a session by id.
  */
@@ -302,19 +308,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 v3 2/2] 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/errcode.h      |  2 +-
 src/box/iproto.cc      |  9 +++++---
 src/box/session.cc     | 23 +++++++++++++++++++
 src/box/session.h      |  7 ++++++
 test/box/misc.result   |  1 +
 test/box/push.result   | 50 ++++++++++++++++++++++++++++++++++++++++++
 test/box/push.test.lua | 22 +++++++++++++++++++
 7 files changed, 110 insertions(+), 4 deletions(-)

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 73359ebdf..9d3e89f85 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -138,7 +138,7 @@ struct errcode_record {
 	/* 83 */_(ER_ROLE_EXISTS,		"Role '%s' already exists") \
 	/* 84 */_(ER_CREATE_ROLE,		"Failed to create role '%s': %s") \
 	/* 85 */_(ER_INDEX_EXISTS,		"Index '%s' already exists") \
-	/* 86 */_(ER_UNUSED6,			"") \
+	/* 86 */_(ER_SESSION_CLOSED,		"Session is closed") \
 	/* 87 */_(ER_ROLE_LOOP,			"Granting role '%s' to role '%s' would create a loop") \
 	/* 88 */_(ER_GRANT,			"Incorrect grant arguments: %s") \
 	/* 89 */_(ER_PRIV_GRANTED,		"User '%s' already has %s access on %s '%s'") \
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 3edfd8f28..1debc3c84 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1246,9 +1246,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_close(con->session);
+		if (! rlist_empty(&session_on_disconnect)) {
+			tx_fiber_init(con->session, 0);
+			session_run_on_disconnect_triggers(con->session);
+		}
 	}
 }
 
diff --git a/src/box/session.cc b/src/box/session.cc
index 0ec118bca..70822e8fd 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -92,10 +92,33 @@ session_on_stop(struct trigger *trigger, void * /* event */)
 	session_destroy(fiber_get_session(fiber()));
 }
 
+static int
+closed_session_push(struct session *session, uint64_t sync, struct port *port)
+{
+	(void) session;
+	(void) sync;
+	(void) port;
+	diag_set(ClientError, ER_SESSION_CLOSED);
+	return -1;
+}
+
+static struct session_vtab closed_session_vtab = {
+	/* .push = */ closed_session_push,
+	/* .fd = */ generic_session_fd,
+	/* .sync = */ generic_session_sync,
+};
+
+void
+session_close(struct session *session)
+{
+	session->vtab = &closed_session_vtab;
+}
+
 void
 session_set_type(struct session *session, enum session_type type)
 {
 	assert(type < session_type_MAX);
+	assert(session->vtab != &closed_session_vtab);
 	session->type = type;
 	session->vtab = &session_vtab_registry[type];
 }
diff --git a/src/box/session.h b/src/box/session.h
index d86939e48..3a7397146 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -146,6 +146,13 @@ extern struct session_vtab session_vtab_registry[];
 void
 session_set_type(struct session *session, enum session_type type);
 
+/**
+ * Close a session. It will return errors from all virtual methods
+ * and its type is fixed.
+ */
+void
+session_close(struct session *session);
+
 /**
  * Find a session by id.
  */
diff --git a/test/box/misc.result b/test/box/misc.result
index d266bb334..9fecbce76 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -415,6 +415,7 @@ t;
   83: box.error.ROLE_EXISTS
   84: box.error.CREATE_ROLE
   85: box.error.INDEX_EXISTS
+  86: box.error.SESSION_CLOSED
   87: box.error.ROLE_LOOP
   88: box.error.GRANT
   89: box.error.PRIV_GRANTED
diff --git a/test/box/push.result b/test/box/push.result
index af730c1a7..8919a3f5b 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 is closed
+...
+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 v3 0/2] Outdate binary session on disconnect

[Vladimir Davydov]

On Fri, Dec 21, 2018 at 07:48:39PM +0300, Vladislav Shpilevoy wrote:
> Vladislav Shpilevoy (2):
>   session: store vtab both in struct and registry
>   session: outdate a session of a closed connection

Pushed to 2.1 and 1.10.