From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
 bits)) (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 38BC24765E0
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 22 Dec 2020 17:21:05 +0300 (MSK)
References: <cover.1608596852.git.i.kosarev@tarantool.org>
 <1e82881e022f12ac8f2381c072459e838eefaff6.1608596852.git.i.kosarev@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <0e17b527-929c-b757-227a-f3b36c7906c5@tarantool.org>
Date: Tue, 22 Dec 2020 15:21:03 +0100
MIME-Version: 1.0
In-Reply-To: <1e82881e022f12ac8f2381c072459e838eefaff6.1608596852.git.i.kosarev@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v7 3/3] iproto: move greeting from
 tx thread to iproto
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Ilya Kosarev <i.kosarev@tarantool.org>, alyapunov@tarantool.org
Cc: tarantool-patches@dev.tarantool.org

Thanks for the patch!

Did you even test it?

I used exactly the same test as in my last email and I still get
"too many open files".

See 2 comments below.

> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index f7330af21d..b48a774c92 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1484,8 +1544,16 @@ static inline struct iproto_msg *
>  tx_accept_msg(struct cmsg *m)
>  {
>  	struct iproto_msg *msg = (struct iproto_msg *) m;
> -	tx_accept_wpos(msg->connection, &msg->wpos);
> -	tx_fiber_init(msg->connection->session, msg->header.sync);
> +	struct iproto_connection *con = msg->connection;
> +	if (con->state != IPROTO_CONNECTION_ALIVE) {

1. Connection state can only be changed and read by iproto thread.
The variable is not protected anyhow, so you can't simply read/write
it in 2 threads.

> +		/*
> +		 * Connection might be closed from iproto already.
> +		 * No action required in this case.
> +		 */
> +		return msg;

2. Why do you return a message instead of NULL here, if the
connection is already dead?

I ask you to spend more time on finishing the patch. If you will
rush to "fix" it without thinking it through, the review will
never end, and it will waste time both yours and mine.