Re: [tarantool-patches] [PATCH v2 08/10] session: introduce text box.session.push

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/04/20 16:31]:
> +/**
> + * Push a tagged YAML document into a console socket.
> + * @param session Console session.
> + * @param port Port with YAML to push.
> + *
> + * @retval  0 Success.
> + * @retval -1 Error.
> + */
> +static int
> +console_session_push(struct session *session, struct port *port)
> +{
> +	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)
> +		return -1;
> +	int fd = session_fd(session);
> +	while (text_len > 0) {
> +		while (coio_wait(fd, COIO_WRITE,
> +				 TIMEOUT_INFINITY) != COIO_WRITE);

Nitpick: the socket is ready in 99% of cases, there is no reason
to call coio_wait() unless you get EINTR. 
Second, why the choice of fio_writev() rather than write() or
send? 

What is wrong with coio_writev or similar?

> +		const struct iovec iov = {
> +			.iov_base = (void *) text,
> +			.iov_len = text_len
> +		};
> +		ssize_t rc = fio_writev(fd, &iov, 1);
> +		if (rc < 0)
> +			return -1;
> +		text_len -= rc;
> +		text += rc;
> +	}
> +	return 0;
> +}
> @@ -92,13 +93,27 @@ local text_connection_mt = {
>          --
>          eval = function(self, text)
>              text = text..'$EOF$\n'
> -            if self:write(text) then
> +            if not self:write(text) then
> +                error(self:set_error())
> +            end
> +            while true do
>                  local rc = self:read()
> -                if rc then
> +                if not rc then
> +                    break
> +                end
> +                local handle, prefix = yaml.decode_tag(rc)
> +                assert(handle or not prefix)
> +                if not handle then
> +                    -- Can not fail - tags are encoded with no
> +                    -- user participation and are correct always.
> +                    assert(not prefix)

In Lua, asserts take CPU time. Please don't use them unless in a
test.

>                      return rc
>                  end
> +                if handle == PUSH_TAG_HANDLE and self.print_f then
> +                    self.print_f(rc)
> +                end
>              end
> -            error(self:set_error())
> +            return rc
>          end,
>          --
>          -- Make the connection be in error state, set error
> @@ -121,15 +136,18 @@ local text_connection_mt = {
>  -- netbox-like object.
>  -- @param connection Socket to wrap.
>  -- @param url Parsed destination URL.
> +-- @param print_f Function to print push messages.
> +--
>  -- @retval nil, err Error, and err contains an error message.
>  -- @retval  not nil Netbox-like object.
>  --
> -local function wrap_text_socket(connection, url)
> +local function wrap_text_socket(connection, url, print_f)
>      local conn = setmetatable({
>          _socket = connection,
>          state = 'active',
>          host = url.host or 'localhost',
>          port = url.service,
> +        print_f = print_f,
>      }, text_connection_mt)
>      if not conn:write('require("console").delimiter("$EOF$")\n') or
>         not conn:read() then
> @@ -369,7 +387,8 @@ local function connect(uri, opts)
>      end
>      local remote
>      if greeting.protocol == 'Lua console' then
> -        remote = wrap_text_socket(connection, u)
> +        remote = wrap_text_socket(connection, u,
> +                                  function(msg) self:print(msg) end)
>      else
>          opts = {
>              connect_timeout = opts.timeout,
> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
> index 5fe5f08d4..306271809 100644
> --- a/src/box/lua/session.c
> +++ b/src/box/lua/session.c
> @@ -41,6 +41,8 @@
>  #include "box/session.h"
>  #include "box/user.h"
>  #include "box/schema.h"
> +#include "box/port.h"
> +#include "box/lua/console.h"
>  
>  static const char *sessionlib_name = "box.session";
>  
> @@ -355,6 +357,52 @@ lbox_push_on_access_denied_event(struct lua_State *L, void *event)
>  	return 3;
>  }
>  
> +/**
> + * Port to push a message from Lua.
> + */
> +struct lua_push_port {
> +	const struct port_vtab *vtab;
> +	/**
> +	 * Lua state, containing data to dump on top of the stack.
> +	 */
> +	struct lua_State *L;
> +};
> +
> +static const char *
> +lua_push_port_dump_plain(struct port *port, uint32_t *size);
> +
> +static const struct port_vtab lua_push_port_vtab = {
> +       .dump_msgpack = NULL,
> +       /*
> +        * Dump_16 has no sense, since push appears since 1.10
> +        * protocol.
> +        */
> +       .dump_msgpack_16 = NULL,
> +       .dump_plain = lua_push_port_dump_plain,
> +       .destroy = NULL,
> +};
> +
> +static const char *
> +lua_push_port_dump_plain(struct port *port, uint32_t *size)
> +{
> +	struct lua_push_port *lua_port = (struct lua_push_port *) port;
> +	assert(lua_port->vtab == &lua_push_port_vtab);
> +	struct lua_State *L = lua_port->L;
> +	int rc = console_encode_push(L);
> +	if (rc == 2) {
> +		assert(lua_isnil(L, -2));
> +		assert(lua_isstring(L, -1));
> +		diag_set(ClientError, ER_PROC_LUA, lua_tostring(L, -1));
> +		return NULL;
> +	}
> +	assert(rc == 1);
> +	assert(lua_isstring(L, -1));
> +	size_t len;
> +	const char *result = lua_tolstring(L, -1, &len);
> +	*size = (uint32_t) len;
> +	return result;
> +}
> +
>  /**
>   * Push a message using a protocol, depending on a session type.
>   * @param data Data to push, first argument on a stack.
> @@ -367,7 +415,11 @@ lbox_session_push(struct lua_State *L)
>  	if (lua_gettop(L) != 1)
>  		return luaL_error(L, "Usage: box.session.push(data)");
>  
> -	if (session_push(current_session(), NULL) != 0) {
> +	struct lua_push_port port;
> +	port.vtab = &lua_push_port_vtab;

Why do you need a separate port? 

And why do you need to create a port for every push? Can't you
reuse the same port as is used for the Lua call itself?

> +	port.L = L;
> +
> +	if (session_push(current_session(), (struct port *) &port) != 0) {
>  		lua_pushnil(L);
>  		luaT_pusherror(L, box_error_last());
>  		return 2;
> diff --git a/src/box/port.c b/src/box/port.c
> index 255eb732c..f9b655840 100644

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