From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 21 Mar 2018 15:25:20 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 5/5] session: introduce box.session.push Message-ID: <20180321122520.5a7pdejytktkfffz@esperanza> References: <20180321091050.cj763i43mbeotxcd@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: "v.shpilevoy@tarantool.org" Cc: tarantool-patches@freelists.org List-ID: On Wed, Mar 21, 2018 at 12:30:19PM +0300, v.shpilevoy@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.