Re: [Tarantool-patches] [PATCH 13/20] net.box: rewrite send_and_recv_{iproto, console} in C

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for working on this!

See 3 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index e88db6323afa..12d82738a050 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -465,62 +465,45 @@ netbox_decode_greeting(lua_State *L)
>   * interaction.
>   */
>  static int
> -netbox_communicate(lua_State *L)
> +netbox_communicate(int fd, struct ibuf *send_buf, struct ibuf *recv_buf,
> +		   size_t limit, const void *boundary, size_t boundary_len,
> +		   double timeout, size_t *limit_or_boundary_pos)

1. IMO, the name looks really bulky. I would just call it `size` or `end_pos`.
Up to you.

>  {
> -	uint32_t fd = lua_tonumber(L, 1);
>  	const int NETBOX_READAHEAD = 16320;
> -	struct ibuf *send_buf = (struct ibuf *) lua_topointer(L, 2);
> -	struct ibuf *recv_buf = (struct ibuf *) lua_topointer(L, 3);
> -
> -	/* limit or boundary */
> -	size_t limit = SIZE_MAX;
> -	const void *boundary = NULL;
> -	size_t boundary_len;
> -
> -	if (lua_type(L, 4) == LUA_TSTRING)
> -		boundary = lua_tolstring(L, 4, &boundary_len);
> -	else
> -		limit = lua_tonumber(L, 4);
> -
> -	/* timeout */
> -	ev_tstamp timeout = TIMEOUT_INFINITY;
> -	if (lua_type(L, 5) == LUA_TNUMBER)
> -		timeout = lua_tonumber(L, 5);
>  	if (timeout < 0) {
> -		lua_pushinteger(L, ER_TIMEOUT);
> -		lua_pushstring(L, "Timeout exceeded");
> -		return 2;
> +		diag_set(ClientError, ER_TIMEOUT);
> +		return -1;
>  	}
>  	int revents = COIO_READ;
>  	while (true) {
>  		/* reader serviced first */
>  check_limit:
>  		if (ibuf_used(recv_buf) >= limit) {
> -			lua_pushnil(L);
> -			lua_pushinteger(L, (lua_Integer)limit);
> -			return 2;
> +			*limit_or_boundary_pos = limit;
> +			return 0;
>  		}
>  		const char *p;
>  		if (boundary != NULL && (p = memmem(
>  					recv_buf->rpos,
>  					ibuf_used(recv_buf),
>  					boundary, boundary_len)) != NULL) {
> -			lua_pushnil(L);
> -			lua_pushinteger(L, (lua_Integer)(
> -					p - recv_buf->rpos));
> -			return 2;
> +			*limit_or_boundary_pos = p - recv_buf->rpos;
> +			return 0;
>  		}
>  
>  		while (revents & COIO_READ) {
>  			void *p = ibuf_reserve(recv_buf, NETBOX_READAHEAD);
> -			if (p == NULL)
> -				luaL_error(L, "out of memory");
> +			if (p == NULL) {
> +				diag_set(OutOfMemory, NETBOX_READAHEAD,
> +					 "ibuf", "recv_buf");

2. For OutOfMemory errors as a rule we use the format

	diag_set(OutOfMemory, size, "name of the allocation function",
	         "name of the variable")

So here it would be

	diag_set(OutOfMemory, NETBOX_READAHEAD, "ibuf_reserve", "p");

I know it is violated in a lot of old code and I gave up trying to
enforce it in new patches to exact that form. Up to you.

The same in the other new OutOfMemory in the other patches.

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index d7394b088752..0ad6cac022f2 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -582,46 +581,23 @@ local function create_transport(host, port, user, password, callback,
>      end
>  
>      -- IO (WORKER FIBER) --
> -    local function send_and_recv(limit_or_boundary, timeout)
> -        return communicate(connection:fd(), send_buf, recv_buf,
> -                           limit_or_boundary, timeout)
> -    end
> -
>      local function send_and_recv_iproto(timeout)
> -        local data_len = recv_buf.wpos - recv_buf.rpos
> -        local required
> -        if data_len < 5 then
> -            required = 5
> -        else
> -            -- PWN! insufficient input validation
> -            local bufpos = recv_buf.rpos
> -            local len, rpos = decode(bufpos)
> -            required = (rpos - bufpos) + len
> -            if data_len >= required then
> -                local body_end = rpos + len
> -                local hdr, body_rpos = decode(rpos)
> -                recv_buf.rpos = body_end
> -                return nil, hdr, body_rpos, body_end
> -            end
> +        local hdr, body_rpos, body_end = internal.send_and_recv_iproto(

3. Indexing 'internal' via '.' is not free. It is a lookup
in a hash. You might want to save internal.send_and_recv_iproto into
a global variable when the module loads first time and use the
cached value. Non-cached version is a bit faster only for FFI, but
here you are using Lua C - cache should be good.

> +            connection:fd(), send_buf, recv_buf, timeout)

Another idea is to cache 'connection:fd()' result into a variable in
the root of create_transport() function. And update it when the
connetion is re-established. Although you probably move this all to
C later as well, I didn't reach the last commits yet.