[tarantool-patches] Re: [PATCH] Pass max_total_connections parameter to httpclient

[Alexander Turenko]

Now docbot comment is added (see [1]), typos are fixed. The code is
okay. LGTM.

Kirill, please, proceed.

https://github.com/tarantool/tarantool/issues/3945
https://github.com/tarantool/tarantool/tree/runsfor/gh-3945-pass-new-opt-to-httpc

[1]: https://github.com/tarantool/tarantool/issues/3945#issuecomment-491539435

On Wed, May 08, 2019 at 06:09:42PM +0300, Alexander Turenko wrote:
> The patch and the description looks okay for me.
> 
> Please, add docbot ( https://github.com/tarantool/docbot ) comment into
> the commit or the issue. I think that certain recommendations for users
> would be valuable result of your investigations.
> 
> Maybe all details you described in the commit message is not worth to
> place in our documentation, but the following points looks important:
> 
> * How to achieve good performance in case of arbitrary host requests and in
>   case of requests to the same small set of hosts: whether it worth to
>   leave default settings or how to calculate good values (maybe just
>   your observations if you don't have strict vision on this).
> * What to check if a performance is degraded or there is such suspicion:
>   at least it worth to mention that CentOS 7 ships curl 7.29 by default
>   (AFAIR): so max_total_connections will be ignored and so we'll fall
>   into the situation with decraged performance you described below.
> 
> See also three comments below.
> 
> WBR, Alexander Turenko.
> 
> > httpc: add MAX_TOTAL_CONNECTIONS option binding
> > 
> > Right now there is only one option which is configurable for http
> > client. That is CURLMOPT_MAXCONNECTS. It can be setup like this:
> > 
> > > httpc = require('http.client').new({max_connections = 16})
> > 
> > Basically, this option tells curl to maintain this many connections in
> > the cache during client instance lifetime. Caching connections are very
> > usefull when user requests mostly same hosts.
> 
> Typo: usefull -> useful.
> 
> > 
> > When connections cache is full and all of them are waiting for response
> > and new request comes in, curl creates a new connection, starts request
> > and then drops first available connection to keep connections cache size
> > right.
> > 
> > There is one side effect, that when tcp connection is closed, system
> > actually updates its state to TIME_WAIT. Then for some time resources
> > for this socket can't be reused (usually 60 seconds).
> > 
> > When user wants to do lots of requests simultaneously (to the same
> > host), curl ends up creating and dropping lots of connections, which is
> > not very efficient. When this load is high enough, sockets won't be able
> > to recover from TIME_WAIT because of timeout and system may run out of
> > available sockets which results in performance reduce. And user right
> > now cannot control or limit this behaviour.
> > 
> > The solution is to add a new binding for CURLMOPT_MAX_TOTAL_CONNECTIONS
> > option. This option tells curl to hold a new connection until
> > there is one available (request is finished). Only after that curl will
> > either drop and create new connection or reuse an old one.
> > 
> > This patch bypasses this option into curl instance. It defaults to -1
> > which means that there is no limit. To create a client with this option
> > setup, user needs to set max_total_connections option like this:
> > 
> > > httpc = require('http.client').new({max_connections = 8,
> >                                       max_total_connections = 8})
> > 
> > In general this options usually usefull when doing requests mostly to
> > the same hosts. Other way, defaults should be enough.
> 
> Typo: usefull -> useful.
> 
> > 
> > Option CURLMOPT_MAX_TOTAL_CONNECTIONS was added from 7.30.0 version, so
> > if curl version is under 7.30.0, this option is simply ignored.
> > https://curl.haxx.se/changes.html#7_30_0
> > 
> > Also, this patch adjusts the default for CURLMOPT_MAX_CONNECTS option to
> > 0 which means that for every new easy handle curl will enlarge its max
> > cache size by 4. See this option docs for more
> > https://curl.haxx.se/libcurl/c/CURLMOPT_MAXCONNECTS.html
> 
> Please add 'Fixes #xxxx' mark or kinda to auto-close the issue.
> 
> On Mon, May 06, 2019 at 06:20:26AM -0700, Илья Конюхов wrote:
> > Hi,
> > 
> > Thanks for your detailed review.
> > 
> > 
> > I added couple simple changes to the patch (see diff from original patch
> > below) and pushed it to the tarantool/tarantool repo. See
> > branch runsfor/gh-3945-pass-new-opt-to-httpc.
> > 
> > Also I had rewrite the whole commit message trying to be more specific.
> > 
> > > 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.
> > 
> > I agree with your comments about options defaults, so I changed defaults to
> > match documentation. I was not able to find any reasons why max_connections
> > option was default to 5. It was originally added in tarantool/curl repo
> > without any comments and then migrate into tarantool.
> > 
> > ---
> > diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> > index ef0efa4bf..50ff91a36 100644
> > --- a/src/lua/httpc.lua
> > +++ b/src/lua/httpc.lua
> > @@ -41,8 +41,8 @@ local curl_mt
> >  --
> >  --  Parameters:
> >  --
> > ---  max_connections -  Maximum number of entries in the connection cache */
> > ---  max_total_connections -  Maximum number of active connections */
> > +--  max_connections -  Maximum number of entries in the connection cache
> > +--  max_total_connections -  Maximum number of active connections
> >  --
> >  --  Returns:
> >  --  curl object or raise error()
> > @@ -52,8 +52,8 @@ local http_new = function(opts)
> > 
> >      opts = opts or {}
> > 
> > -    opts.max_connections = opts.max_connections or 8
> > -    opts.max_total_connections = opts.max_total_connections or 8
> > +    opts.max_connections = opts.max_connections or -1
> > +    opts.max_total_connections = opts.max_total_connections or 0
> > 
> >      local curl = driver.new(opts.max_connections,
> > opts.max_total_connections)
> >      return setmetatable({ curl = curl, }, curl_mt )
> >