[tarantool-patches] Re: [PATCH 5/5] session: introduce box.session.push

[v.shpilevoy at tarantool.org]

> 
> I doubt we need to designate text pushes at all. IMO they are useful
> only for printing text to the user console. I suggest you disable
> the on_push callback if net_box is operating in the 'console' mode,
> instead just append pushes to the output, without a prefix.

On_push can be used from netbox, where pushed message will finish request, if it has no
prefix - it just can not be distinguished from a final response.

> 
>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>> +/**
>> + * Search for IPROTO_PUSH key in a MessagePack encoded response
>> + * body. It is needed without entire message decoding, when a user
>> + * wants to store raw responses and pushes in its own buffer.
>> + */
>> +static int
>> +netbox_body_is_push(struct lua_State *L)
>> +{
>> +	uint32_t ctypeid;
>> +	const char *body = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
>> +	assert(ctypeid == luaL_ctypeid(L, "char *"));
>> +	assert(mp_typeof(*body) == MP_MAP);
>> +	lua_pushboolean(L, mp_decode_map(&body) == 1 &&
>> +			   mp_typeof(*body) == MP_UINT &&
>> +			   mp_decode_uint(&body) == IPROTO_PUSH);
>> +	return 1;
>> +}
>> +
> 
> Can't you do this check in net_box.lua, without involving C?

No, messagepack Lua api does not allow to decode a part of a message.

> 
>> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
>> +/**
>> + * Write @a text into @a fd in a blocking mode, ignoring transient
>> + * socket errors.
>> + * @param fd Console descriptor.
>> + * @param text Text to send.
>> + * @param len Length of @a text.
>> + */
>> +static inline int
>> +console_do_push(int fd, const char *text, uint32_t len)
>> +{
>> +	while (len > 0) {
>> +		int written = fio_write_silent(fd, text, len);
> 
> I don't think that using a blocking function here is acceptable
> (AFAICS fio_write_silent() calls the write syscall on session fd).

It is acceptable since it is blocking in the original Lua code. See
console.lua and socket.lua.
 
>> 	return 0;
>> +}
> 
>> +/**
>> + * Push a message using a protocol, depending on a session type.
>> + * @param data Data to push, first argument on a stack.
>> + * @param opts Options. Now requires a single possible option -
>> + *        sync. Second argument on a stack.
>> + */
>> +static int
>> +lbox_session_push(struct lua_State *L)
>> +{
>> +	if (lua_gettop(L) != 2 || !lua_istable(L, 2)) {
>> +usage_error:
>> +		return luaL_error(L, "Usage: box.session.push(data, opts)");
> 
> I don't think that we should oblige the user to pass the 'sync' value
> explicitly - this would be really annoying. I think we should save the
> sync somehow (fiber local storage, request?) and pass it implicitly.

In the push ticket on Github you can see a discussion about this question. And it appeared, that passing sync explicitly is the only way to do push correctly. We can store it neither in fiber (encapsulation violation) nor in Lua in some hidden variable (a tried, many many times) nor use session sync (it can be changed after yield). So passing sync explicitly is ok. It is approved by Kostja.

>> +	}
>> +	lua_getfield(L, 2, "sync");
>> +	if (! lua_isnumber(L, 3))
>> +		goto usage_error;
>> +	double lua_sync = lua_tonumber(L, 3);
>> +	lua_pop(L, 1);
>> +	uint64_t sync = (uint64_t) lua_sync;
>> +	if (lua_sync != sync)
>> +		goto usage_error;
>> +	struct lua_push_port port;
>> +	port.vtab = &lua_push_port_vtab;
>> +	port.L = L;
>> +	/*
>> +	 * Pop the opts - they must not be pushed. Leave only data
>> +	 * on a stack.
>> +	 */
>> +	lua_remove(L, 2);
>> +	if (session_push(current_session(), sync, (struct port *) &port) != 0) {
>> +		return luaT_error(L);
>> +	} else {
>> +		lua_pushboolean(L, true);
>> +		return 1;
> 
> What's the point in returning 'true' on success?

What is alternative?

> 
>> +	}
>> +}
> 
>> diff --git a/src/box/port.h b/src/box/port.h
>> @@ -76,6 +76,11 @@ struct port_vtab {
>> 	 * format.
>> 	 */
>> 	int (*dump_16)(struct port *port, struct obuf *out);
>> +	/**
>> +	 * Same as dump, but find a memory for an output buffer
>> +	 * for itself.
>> +	 */
>> +	const char *(*dump_raw)(struct port *port, uint32_t *size);
> 
> Somehow this doesn't feel right. May be, we should encode Lua stack in
> msgpack first, and then re-encode it to Yaml. May be, we shouldn't use
> the 'port' at all. May be, I'm being too picky, and we should leave it
> as is. Anyway, please think of alternatives.

I already have spent many time on alternatives, and it appeared, that a port
is the most useful way. Formatting to a message pack and back to YAML is memory
and CPU overhead, and moreover if we format it in a message pack, we are forced to
use region for encoded data, because console has no obuf, and into IProto this region
must be copied. Now for IProto pushes a message is encoded directly in obuf, with no
multiple coping.

> 
>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>> @@ -43,6 +43,9 @@
>> #include "scramble.h"
>> #include "iproto_constants.h"
>> 
>> +static_assert(IPROTO_DATA < 0x7f && IPROTO_PUSH < 0x7f,
>> +	      "encoded IPROTO_BODY keys must fit into one byte");
>> +
> 
> Why check this now? IPROTO_DATA and IPROTO_PUSH can't occasionally
> change as they are defined in the binary protocol so there's no point
> in this static assertion IMO.

I thought that it looks more clear for a newbie, who reads IProto code. For me at the beginning it
was very unclear why we are sure, that codes can fit into one byte, and how iproto_body/header_bin
work.
But if you think, that it is bad idea, I can delete it.

> 
>> int
>> xrow_header_decode(struct xrow_header *header, const char **pos,
>> 		   const char *end)
>> @@ -231,6 +234,9 @@ struct PACKED iproto_body_bin {
>> 	uint32_t v_data_len;               /* string length of array size */
>> };
>> 
>> +static_assert(sizeof(struct iproto_body_bin) + IPROTO_HEADER_LEN ==
>> +	      IPROTO_SELECT_HEADER_LEN, "size of the prepared select");
>> +
>> static const struct iproto_body_bin iproto_body_bin = {
>> 	0x81, IPROTO_DATA, 0xdd, 0
>> };
>> @@ -239,6 +245,19 @@ static const struct iproto_body_bin iproto_error_bin = {
>> 	0x81, IPROTO_ERROR, 0xdb, 0
>> };
>> 
>> +struct PACKED iproto_body_push_bin {
>> +	uint8_t m_body;       /* MP_MAP */
>> +	uint8_t k_data;       /* IPROTO_PUSH */
>> +	uint8_t v_data;       /* 1-size MP_ARRAY */
>> +};
> 
> Why don't you just reuse iproto_body_bin for this?

IProto push message body header requires just 3 bytes, while response body requires 7.

>