[Tarantool-patches] [PATCH 1/1] session: remove box.session.push() 'sync'

[Tarantool-patches] [PATCH 1/1] session: remove box.session.push() 'sync'

[Vladislav Shpilevoy]

Closes #4689

@TarantoolBot document
Title: box.session.push() 'sync' is deprecated

box.session.push() had two parameters - data to push and 'sync'.
The sync was a request ID with which the out of bound data should
be pushed into a socket.

This was introduced as a workaround for #3450, and is useless
since its resolution.

A user anyway can't push to different sessions, where that
parameter could be useful. And pushing into requests of the same
session, on the contrary, is something not really needed anywhere,
not portable to non-binary session types (console, background),
and is just dangerous since it is easy to add a bug here.

The patch removes the parameter. Now there will be thrown a
'Usage' error at attempt to use 'sync' parameter. In version 2.4
it is deprecated, prints warnings into logs, but still works. In
2.5 it is removed completely.
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4689-push-sync-remove-2.5
Issue: https://github.com/tarantool/tarantool/issues/4689

The commit is for master branch only.

 src/box/box.cc                |  2 +-
 src/box/iproto.cc             |  6 +++---
 src/box/lua/console.c         |  4 +---
 src/box/lua/session.c         | 20 +++-----------------
 src/box/session.cc            |  6 ++----
 src/box/session.h             | 10 ++++------
 test/app-tap/console.test.lua | 10 +---------
 test/box/push.result          | 18 ++++++++----------
 test/box/push.test.lua        | 14 ++++++--------
 9 files changed, 29 insertions(+), 61 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index ae0907e0f..8b58e6859 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1470,7 +1470,7 @@ box_session_push(const char *data, const char *data_end)
 	struct port_msgpack port;
 	struct port *base = (struct port *)&port;
 	port_msgpack_create(base, data, data_end - data);
-	int rc = session_push(session, session_sync(session), base);
+	int rc = session_push(session, base);
 	port_msgpack_destroy(base);
 	return rc;
 }
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index 9dad43b0b..51fe7d93b 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -2153,7 +2153,6 @@ tx_end_push(struct cmsg *m)
 /**
  * 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.
@@ -2162,7 +2161,7 @@ tx_end_push(struct cmsg *m)
  *            client: the output buffer is flushed asynchronously.
  */
 static int
-iproto_session_push(struct session *session, uint64_t sync, struct port *port)
+iproto_session_push(struct session *session, struct port *port)
 {
 	struct iproto_connection *con =
 		(struct iproto_connection *) session->meta.connection;
@@ -2173,7 +2172,8 @@ iproto_session_push(struct session *session, uint64_t sync, struct port *port)
 		obuf_rollback_to_svp(con->tx.p_obuf, &svp);
 		return -1;
 	}
-	iproto_reply_chunk(con->tx.p_obuf, &svp, sync, ::schema_version);
+	iproto_reply_chunk(con->tx.p_obuf, &svp, iproto_session_sync(session),
+			   ::schema_version);
 	if (! con->tx.is_push_sent)
 		tx_begin_push(con);
 	else
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index b941f50c6..666151084 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -510,16 +510,14 @@ port_msgpack_dump_plain(struct port *base, uint32_t *size)
  * Push a tagged YAML document or a Lua string into a console
  * socket.
  * @param session Console session.
- * @param sync Unused request sync.
  * @param port Port with the data to push.
  *
  * @retval  0 Success.
  * @retval -1 Error.
  */
 static int
-console_session_push(struct session *session, uint64_t sync, struct port *port)
+console_session_push(struct session *session, struct port *port)
 {
-	(void) sync;
 	assert(session_vtab_registry[session->type].push ==
 	       console_session_push);
 	uint32_t text_len;
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 64453463a..0a20aaad1 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -378,25 +378,11 @@ static int
 lbox_session_push(struct lua_State *L)
 {
 	struct session *session = current_session();
-	uint64_t sync;
-	switch(lua_gettop(L)) {
-	case 1:
-		sync = session_sync(session);
-		break;
-	case 2:
-		sync = luaL_touint64(L, 2);
-		if (sync != 0 || (lua_isnumber(L, 2) &&
-				  lua_tonumber(L, 2) == 0)) {
-			lua_pop(L, 1);
-			break;
-		}
-		FALLTHROUGH;
-	default:
-		return luaL_error(L, "Usage: box.session.push(data, sync)");
-	}
+	if (lua_gettop(L) != 1)
+		return luaL_error(L, "Usage: box.session.push(data)");
 	struct port port;
 	port_lua_create(&port, L);
-	if (session_push(session, sync, &port) != 0)
+	if (session_push(session, &port) != 0)
 		return luaT_push_nil_and_error(L);
 	lua_pushboolean(L, true);
 	return 1;
diff --git a/src/box/session.cc b/src/box/session.cc
index 08a10924a..7085cc393 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -96,10 +96,9 @@ session_on_stop(struct trigger *trigger, void * /* event */)
 }
 
 static int
-closed_session_push(struct session *session, uint64_t sync, struct port *port)
+closed_session_push(struct session *session, struct port *port)
 {
 	(void) session;
-	(void) sync;
 	(void) port;
 	diag_set(ClientError, ER_SESSION_CLOSED);
 	return -1;
@@ -354,10 +353,9 @@ access_check_universe(user_access_t access)
 }
 
 int
-generic_session_push(struct session *session, uint64_t sync, struct port *port)
+generic_session_push(struct session *session, struct port *port)
 {
 	(void) port;
-	(void) sync;
 	const char *name =
 		tt_sprintf("Session '%s'", session_type_strs[session->type]);
 	diag_set(ClientError, ER_UNSUPPORTED, name, "push()");
diff --git a/src/box/session.h b/src/box/session.h
index 500a88b22..a15434f96 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -130,15 +130,13 @@ struct session_vtab {
 	 * Push a port data into a session data channel - socket,
 	 * console or something.
 	 * @param session Session to push into.
-	 * @param sync Request sync in scope of which to send the
-	 *        push.
 	 * @param port Port with data to push.
 	 *
 	 * @retval  0 Success.
 	 * @retval -1 Error.
 	 */
 	int
-	(*push)(struct session *session, uint64_t sync, struct port *port);
+	(*push)(struct session *session, struct port *port);
 	/**
 	 * Get session file descriptor if exists.
 	 * @param session Session to get descriptor from.
@@ -345,9 +343,9 @@ access_check_universe_object(user_access_t access,
 			     const char *object_name);
 
 static inline int
-session_push(struct session *session, uint64_t sync, struct port *port)
+session_push(struct session *session, struct port *port)
 {
-	return session->vtab->push(session, sync, port);
+	return session->vtab->push(session, port);
 }
 
 static inline int
@@ -367,7 +365,7 @@ session_sync(struct session *session)
  * function always returns -1 and sets ER_UNSUPPORTED error.
  */
 int
-generic_session_push(struct session *session, uint64_t sync, struct port *port);
+generic_session_push(struct session *session, struct port *port);
 
 /** Return -1 from any session. */
 int
diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index 4feadfa5e..d5452aa17 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(80)
+test:plan(78)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -75,14 +75,6 @@ test:is(client:read(';'), '"res";', 'returned result')
 client:write('\\set output yaml\n')
 client:read(EOL)
 
---
--- gh-3790: box.session.push support uint64_t sync.
---
-client:write('box.session.push(1, 9223372036854775808ULL)\n')
-test:is(client:read(EOL), '%TAG !push! tag:tarantool.io/push,2018\n--- 1\n...\n',
-        "pushed message")
-test:is(client:read(EOL), '---\n- true\n...\n', "pushed message")
-
 -- Execute some command
 client:write("1\n")
 test:is(yaml.decode(client:read(EOL))[1], 1, "eval")
diff --git a/test/box/push.result b/test/box/push.result
index 7888e4d92..8a734e261 100644
--- a/test/box/push.result
+++ b/test/box/push.result
@@ -6,14 +6,15 @@ test_run = require('test_run').new()
 --
 --
 -- Usage.
+-- gh-4689: sync is deprecated, accepts only 'data' parameter.
 --
 box.session.push()
 ---
-- error: 'Usage: box.session.push(data, sync)'
+- error: 'Usage: box.session.push(data)'
 ...
 box.session.push(1, 'a')
 ---
-- error: 'Usage: box.session.push(data, sync)'
+- error: 'Usage: box.session.push(data)'
 ...
 fiber = require('fiber')
 ---
@@ -95,20 +96,18 @@ test_run:cmd("setopt delimiter ';'")
 - true
 ...
 function dml_push_and_dml(key)
-    local sync = box.session.sync()
-    box.session.push('started dml', sync)
+    box.session.push('started dml')
     s:replace{key}
-    box.session.push('continued dml', sync)
+    box.session.push('continued dml')
     s:replace{-key}
-    box.session.push('finished dml', sync)
+    box.session.push('finished dml')
     return key
 end;
 ---
 ...
 function do_pushes(val)
-    local sync = box.session.sync()
     for i = 1, 5 do
-        box.session.push(i, sync)
+        box.session.push(i)
         fiber.yield()
     end
     return val
@@ -374,9 +373,8 @@ test_run:cmd("setopt delimiter ';'")
 - true
 ...
 function do_pushes()
-    local sync = box.session.sync()
     for i = 1, 5 do
-        box.session.push(i + 100, sync)
+        box.session.push(i + 100)
         cond:wait()
     end
     return true
diff --git a/test/box/push.test.lua b/test/box/push.test.lua
index f4518466e..a66d855a0 100644
--- a/test/box/push.test.lua
+++ b/test/box/push.test.lua
@@ -5,6 +5,7 @@ test_run = require('test_run').new()
 
 --
 -- Usage.
+-- gh-4689: sync is deprecated, accepts only 'data' parameter.
 --
 box.session.push()
 box.session.push(1, 'a')
@@ -44,18 +45,16 @@ pk = s:create_index('pk')
 c:reload_schema()
 test_run:cmd("setopt delimiter ';'")
 function dml_push_and_dml(key)
-    local sync = box.session.sync()
-    box.session.push('started dml', sync)
+    box.session.push('started dml')
     s:replace{key}
-    box.session.push('continued dml', sync)
+    box.session.push('continued dml')
     s:replace{-key}
-    box.session.push('finished dml', sync)
+    box.session.push('finished dml')
     return key
 end;
 function do_pushes(val)
-    local sync = box.session.sync()
     for i = 1, 5 do
-        box.session.push(i, sync)
+        box.session.push(i)
         fiber.yield()
     end
     return val
@@ -191,9 +190,8 @@ c = netbox.connect(box.cfg.listen)
 cond = fiber.cond()
 test_run:cmd("setopt delimiter ';'")
 function do_pushes()
-    local sync = box.session.sync()
     for i = 1, 5 do
-        box.session.push(i + 100, sync)
+        box.session.push(i + 100)
         cond:wait()
     end
     return true
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] session: remove box.session.push() 'sync'

[Vladislav Shpilevoy]

I rebased the branch on master.

(gerold103/gh-4689-push-sync-remove-2.5)

Re: [Tarantool-patches] [PATCH 1/1] session: remove box.session.push() 'sync'

[Kirill Yukhin]

Hello,

On 04 май 23:42, Vladislav Shpilevoy wrote:
> Closes #4689
> 
> @TarantoolBot document
> Title: box.session.push() 'sync' is deprecated
> 
> box.session.push() had two parameters - data to push and 'sync'.
> The sync was a request ID with which the out of bound data should
> be pushed into a socket.
> 
> This was introduced as a workaround for #3450, and is useless
> since its resolution.
> 
> A user anyway can't push to different sessions, where that
> parameter could be useful. And pushing into requests of the same
> session, on the contrary, is something not really needed anywhere,
> not portable to non-binary session types (console, background),
> and is just dangerous since it is easy to add a bug here.
> 
> The patch removes the parameter. Now there will be thrown a
> 'Usage' error at attempt to use 'sync' parameter. In version 2.4
> it is deprecated, prints warnings into logs, but still works. In
> 2.5 it is removed completely.

I've checked your patch into master.

--
Regards, Kirill Yukhin