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 0C39F2BBC3 for ; Sun, 12 May 2019 10:17:12 -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 9jNvgnNN2ilu for ; Sun, 12 May 2019 10:17:11 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 B434C2BBB2 for ; Sun, 12 May 2019 10:17:11 -0400 (EDT) Date: Sun, 12 May 2019 17:16:54 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] Pass max_total_connections parameter to httpclient Message-ID: <20190512141654.vsgn35x4gns6e3bb@tkn_work_nb> References: <20190417145139.59554-1-runsfor@gmail.com> <20190422023809.rip63gv6sqejlojq@tkn_work_nb> <20190508150942.hqyoquiclpgryd4m@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190508150942.hqyoquiclpgryd4m@tkn_work_nb> 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: Kirill Yukhin Cc: tarantool-patches@freelists.org, =?utf-8?B?0JjQu9GM0Y8g0JrQvtC90Y7RhdC+0LI=?= 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 ) > >