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

[Vladimir Davydov]

On Wed, Mar 21, 2018 at 12:30:19PM +0300, v.shpilevoy at tarantool.org wrote:
> 
> > 
> > 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.

For the record: Kostja suggested to use Yaml tags for this.

> 
> > 
> >> 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.

OK.

> 
> > 
> >> 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.

OK.

>  
> >> 	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.

But it's ugly... 'sync' is a part of the protocol, it shouldn't be
exposed to the user. Still I think we'd better use a fiber-local
variable for this.

> 
> >> +	}
> >> +	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?

Return nothing.

> 
> > 
> >> +	}
> >> +}
> > 
> >> 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.

What if we implemented yaml_stream, similar to mpstream we use for
encoding Lua objects in MsgPack, and use it here. Then all port methods
would look consistent.

Anyway, dump_raw is apparently not a very good name, because the
function actually encodes the result in Yaml. What about dump_plain?
Also, the comment is misleading - dump and dump_raw are quite different,
not only by the way they allocate buffer, but also how they present the
result.

> 
> > 
> >> 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.

I like when a patch has as small of a footprint as possible - it's easer
to review it then. So please remove.

> 
> > 
> >> 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.

I don't think these 4 bytes per push are worth the extra code.