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 0E5FA2809B for ; Sun, 21 Apr 2019 22:38:21 -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 QsCU3drLnlIV for ; Sun, 21 Apr 2019 22:38:20 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6C7482771D for ; Sun, 21 Apr 2019 22:38:20 -0400 (EDT) Date: Mon, 22 Apr 2019 05:38:10 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] Pass max_total_connections parameter to httpclient Message-ID: <20190422023809.rip63gv6sqejlojq@tkn_work_nb> References: <20190417145139.59554-1-runsfor@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190417145139.59554-1-runsfor@gmail.com> 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: Ilya Konyukhov Cc: tarantool-patches@freelists.org Thanks for the investigation and the patch! I propose to polish the commit message to make your results more clear for anybody who will be interested and my review is mostly about the wording. Re process: It is better to create a branch within tarantool/tarantool repository named nickname/gh-xxxx-short-description. (It will be easier to pull your changes or push some fixups upward.) Also, please, add my @tarantool.org email to To or CC when ask for review; then I'll not miss your email. Re code: It is okay, but several things need to be clarified (in the commit message). First, why do you change max_connections from 5 to 8? I'm not against, but any change should have a clear reason. It also worth to mention that it possibly was set to 5 by mistake, because the default value of another option (CURLOPT_MAXCONNECTS) is 5. Second. AFAIU, libcurl connections caching is useful when requests are typically going to the same hosts. So I think we need to state explicitly that we assume this case before show calculations about amount of TIME_WAIT sockets and so on. It also worth to mention that tarantool's http client configures libcurl to optimize the case when connections are mainly going to the same hosts and show the way to configure it for requests for arbitrary hosts: set max_connections to -1 and max_total_connections to 0 (see lib/multi.c in curl sources). > There are some usecases when current http client may leak sockets (due > TIME_WAIT timeout). The reason is that the `max_connection` option of > tarantool's http client (actually is a binding for CURLMOPT_MAXCONNECTS > option in libcurl) may not be enough for some situations. Typo: usecases -> use cases. 'Leak' is not accurate term. 'opens many new connections and eventually stuck when exhausted available ports range' -- is this better? However AFAIR the problem that was arose in the issue in not about TIME_WAIT sockets. Correct me if I'm wrong, but it seems that we should describe these two problems separatelly. I understand that in this paragraph you try to describe thing 'in common', but here a reader will have many questions: 'some use cases' -- which? 'may not be enough' -- for what? 'some situations' -- which? If you need to describe how curl operates with cached and non-cached requests with CURLMOPT_MAXCONNECTS and CURLMOPT_MAX_TOTAL_CONNECTIONS options set to specific values then do it first. When you make this clear it would be easy to describe which problems are possible and how they can be handled using max_connections and max_total_connections options. My common suggestions when writing technical texts are try hard to avoid non-specific descriptions ('some' word is the red flag) and avoid forward references. > This option sets up a size of connection cache, which curl maintains > during its lifetime. When this cache is full (i.e. all connections are > waiting for a response from a server), newly created request would > create a new connection to handle it. In that case right after old > connection got response, curl has to close that connection to keep up an > appropriate number of connections in cache. That results in a new socket > in TIME_WAIT state (for 60 seconds by default). > > This is actually easy to calculate. Imagine we have http client with > default 8 maximum connections in cache. Also lets say we have 16384 > sockets available in the system. We need to make requests to external > system which responses in 100ms in average. So, if we start making > request every 11ms (100/9), every finished request will be followed by > curl closing that socket (switches to TIME_WAIT). Result is about 90 > wasted sockets per second and additional overhead for socket creation > for each new request. > > Also if we do more than 274 requests per second, we will likely to be > out of available sockets in 1 minute. > > The solution is to add another binding for max_total_connections option, > which will let curl hold new requests, before old ones have finished. > It also adds more control over sockets resources. I don't see this description in the commit message on your branch. Forgot to update? Also we need to add a docbot request (see https://github.com/tarantool/docbot) and it would be good to move this description into the request to pass it to the documentation team when the commit will land to the master. > > - > > New opt is ignored if curl version is lower 7.30.0 > Option CURLMOPT_MAX_TOTAL_CONNECTIONS was added from 7.30.0 version > https://curl.haxx.se/changes.html#7_30_0 > > - > > Fixes #3945 > > https://github.com/RunsFor/tarantool/tree/master > http://github.com/tarantool/tarantool/issues/3945 Place a text that in not part of a commit message under '---' mark. It will allow to apply your commit with `git am` and, more important, don't confuse a reviewer what is part of a commit message and what is not. > diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua > index cd44b6054..8566db88d 100644 > --- a/src/lua/httpc.lua > +++ b/src/lua/httpc.lua > @@ -41,7 +41,8 @@ local curl_mt > -- > -- Parameters: > -- > --- max_connectionss - Maximum number of entries in the connection cache */ > +-- max_connections - Maximum number of entries in the connection cache */ > +-- max_total_connections - Maximum number of active connections */ '*/' here is just the typo.