From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C96092BD68 for ; Wed, 21 Mar 2018 05:30:23 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4enRb0EUZTZC for ; Wed, 21 Mar 2018 05:30:23 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id ED56A2BD6B for ; Wed, 21 Mar 2018 05:30:22 -0400 (EDT) Received: from [185.6.245.156] (port=54214 helo=msk-wire-v_shpilevoy-v.shpilevoy.mail.msk) by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1eya4a-0002EX-GH for tarantool-patches@freelists.org; Wed, 21 Mar 2018 12:30:20 +0300 From: "v.shpilevoy@tarantool.org" Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 5/5] session: introduce box.session.push Date: Wed, 21 Mar 2018 12:30:19 +0300 References: <20180321091050.cj763i43mbeotxcd@esperanza> In-Reply-To: <20180321091050.cj763i43mbeotxcd@esperanza> Message-Id: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org >=20 > 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. >=20 >> 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 =3D *(const char **)luaL_checkcdata(L, 1, = &ctypeid); >> + assert(ctypeid =3D=3D luaL_ctypeid(L, "char *")); >> + assert(mp_typeof(*body) =3D=3D MP_MAP); >> + lua_pushboolean(L, mp_decode_map(&body) =3D=3D 1 && >> + mp_typeof(*body) =3D=3D MP_UINT && >> + mp_decode_uint(&body) =3D=3D IPROTO_PUSH); >> + return 1; >> +} >> + >=20 > 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. >=20 >> 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 =3D fio_write_silent(fd, text, len); >=20 > 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. =20 >> return 0; >> +} >=20 >> +/** >> + * 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) !=3D 2 || !lua_istable(L, 2)) { >> +usage_error: >> + return luaL_error(L, "Usage: box.session.push(data, = opts)"); >=20 > 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 =3D lua_tonumber(L, 3); >> + lua_pop(L, 1); >> + uint64_t sync =3D (uint64_t) lua_sync; >> + if (lua_sync !=3D sync) >> + goto usage_error; >> + struct lua_push_port port; >> + port.vtab =3D &lua_push_port_vtab; >> + port.L =3D 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) = !=3D 0) { >> + return luaT_error(L); >> + } else { >> + lua_pushboolean(L, true); >> + return 1; >=20 > What's the point in returning 'true' on success? What is alternative? >=20 >> + } >> +} >=20 >> 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); >=20 > 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. >=20 >> diff --git a/src/box/xrow.c b/src/box/xrow.c >> @@ -43,6 +43,9 @@ >> #include "scramble.h" >> #include "iproto_constants.h" >>=20 >> +static_assert(IPROTO_DATA < 0x7f && IPROTO_PUSH < 0x7f, >> + "encoded IPROTO_BODY keys must fit into one byte"); >> + >=20 > 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. >=20 >> 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 */ >> }; >>=20 >> +static_assert(sizeof(struct iproto_body_bin) + IPROTO_HEADER_LEN =3D=3D= >> + IPROTO_SELECT_HEADER_LEN, "size of the prepared select"); >> + >> static const struct iproto_body_bin iproto_body_bin =3D { >> 0x81, IPROTO_DATA, 0xdd, 0 >> }; >> @@ -239,6 +245,19 @@ static const struct iproto_body_bin = iproto_error_bin =3D { >> 0x81, IPROTO_ERROR, 0xdb, 0 >> }; >>=20 >> +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 */ >> +}; >=20 > 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. >=20