Re: патч для Connection: Keep-Alive

[Илья Шипицин]

не слать "Connection: Keep-Alive" для HTTP/1.1 - статистически то же
самое, что я предлагал (ввиду малораспространенности
не-кипэлайв-соединений)

давайте так и сделаем ?

вариант "не слать Connection никогда" мы на кошках проверим, потом расскажу.

14 мая 2013 г., 22:17 пользователь Maxim Dounin <mdounin at mdounin.ru> написал:
> Hello!
>
> On Tue, May 14, 2013 at 09:39:42PM +0600, Илья Шипицин wrote:
>
>> 14 мая 2013 г., 21:16 пользователь Maxim Dounin <mdounin at mdounin.ru> написал:
>> > Hello!
>> >
>> > On Tue, May 14, 2013 at 06:15:05PM +0600, Илья Шипицин wrote:
>> >
>> > [...]
>> >
>> >> во всех случаях, когда добавлялся Keep-Alive: timeout, добавлялся
>> >> также и Connection. не вижу ничего некорректного. можете уточнить ?
>> >>
>> >> другое дело, что я накосячил и не добавлял Keep-Alive: timeout там,
>> >> где предполагалось. да, признаю ошибку. вложил новый патч.
>> >
>> > Это я не досмотрел патч, и предположил не ту ошибку из двух
>> > возможных.
>> >
>> > [...]
>> >
>> >> --- src/http/ngx_http_header_filter_module.c.orig       Tue May 14 12:17:59 2013
>> >> +++ src/http/ngx_http_header_filter_module.c    Tue May 14 16:04:29 2013
>> >> @@ -382,7 +382,7 @@
>> >>      if (r->headers_out.status == NGX_HTTP_SWITCHING_PROTOCOLS) {
>> >>          len += sizeof("Connection: upgrade" CRLF) - 1;
>> >>
>> >> -    } else if (r->keepalive) {
>> >> +    } else if ((r->keepalive) && ((r->http_version == NGX_HTTP_VERSION_10) || (clcf->keepalive_header)) ) {
>> >>          len += sizeof("Connection: keep-alive" CRLF) - 1;
>> >>
>> >>          /*
>> >> @@ -397,9 +397,7 @@
>> >>              len += sizeof("Keep-Alive: timeout=") - 1 + NGX_TIME_T_LEN + 2;
>> >>          }
>> >>
>> >> -    } else {
>> >> -        len += sizeof("Connection: close" CRLF) - 1;
>> >> -    }
>> >> +    }
>> >
>> > "Connection: close" при выключенном keepalive'е нужно указывать,
>> > не возвращать соответствующий заголовок - чревато ненужными
>> > проблемами и безуспешными попытками клиентов послать в то же
>> > соединение следующий запрос.
>> >
>>
>> я понимаю ваши опасения, однако с точки зрения буквы закона, rfc 2616
>>
>> 8.1.4 Practical Considerations
>>
>>
>>  ........
>>
>>    A client, server, or proxy MAY close the transport connection at any
>>    time.
>>  .........
>>
>>
>>
>
> Это _можно_ делать, но на практике - это приводит к сильно
> неоптимальному поведению клиентов.  Поэтому убирать "Connection:
> close" из ответов, keepalive в которых не предполагается  - это
> плохая идея.
>
> С учётом того, что keepalive в нормальных условиях влючён чуть
> более, чем всегда - убирать "Connection: close" не представляется
> разумным ни с какой точки зрения.
>
>> с точки зрения духа закона так поступает IIS (по данным Netcraft-а 16%
>> сайтов), как-то же это работает ...
>
> Специально сходил проверил на первый попавшийся сайт с IIS'ом -
> _так_ IIS, насколько я вижу, не поступает:
>
> $ telnet W3schools.com 80
> Trying 66.29.212.73...
> Connected to W3schools.com.
> Escape character is '^]'.
> HEAD / HTTP/1.1
> Host: w3scools.com
>
> HTTP/1.1 200 OK
> Cache-Control: private
> Content-Length: 29839
> Content-Type: text/html
> Server: Microsoft-IIS/7.5
> Set-Cookie: ASPSESSIONIDCABDDTCD=OMAGHFBBMDCIEKCKOIEGKPAO; path=/
> X-Powered-By: ASP.NET
> Date: Tue, 14 May 2013 16:10:08 GMT
>
> HEAD / HTTP/1.1
> Host: w3scools.com
> Connection: close
>
> HTTP/1.1 200 OK
> Cache-Control: private
> Content-Length: 29839
> Content-Type: text/html
> Server: Microsoft-IIS/7.5
> Set-Cookie: ASPSESSIONIDCABDDTCD=MICGHFBBKNHMJHAFKDAJJOJD; path=/
> X-Powered-By: ASP.NET
> Date: Tue, 14 May 2013 16:10:28 GMT
> Connection: close
>
> Connection closed by foreign host.
>
> Т.е. если соединение закрывается - то "Connection: close" в ответе
> присутствует.
>
>> у нас 50 миллионов запросов в день. это в районе 1 гигабайта на
>> хедерах "Connection:". нам эта цифра интересна.
>> мы, пожалуй, потестируем, всех желающих тоже приглашаю к тесту.
>>
>>
>> > Вообще я бы, честно говоря, не трогал это место.
>>
>> это абстрактные опасения или вы знаете сценарии, при которых это
>> приведет к проблемам ?
>
> При правильной реализации - т.е. не слать "Connection: keep-alive"
> для соединений по HTTP/1.1, если заголовок Keep-Alive не отдаётся -
> только абстрактные.  Все изложенные выше замечания - вполне
> конкретные.
>
> Just in case, правильный патч какой-то такой:
>
> --- a/src/http/ngx_http_header_filter_module.c
> +++ b/src/http/ngx_http_header_filter_module.c
> @@ -383,7 +383,10 @@ ngx_http_header_filter(ngx_http_request_
>          len += sizeof("Connection: upgrade" CRLF) - 1;
>
>      } else if (r->keepalive) {
> -        len += sizeof("Connection: keep-alive" CRLF) - 1;
> +
> +        if (r->http_version < NGX_HTTP_VERSION_11 || clcf->keepalive_header) {
> +            len += sizeof("Connection: keep-alive" CRLF) - 1;
> +        }
>
>          /*
>           * MSIE and Opera ignore the "Keep-Alive: timeout=<N>" header.
> @@ -556,8 +559,11 @@ ngx_http_header_filter(ngx_http_request_
>                               sizeof("Connection: upgrade" CRLF) - 1);
>
>      } else if (r->keepalive) {
> -        b->last = ngx_cpymem(b->last, "Connection: keep-alive" CRLF,
> -                             sizeof("Connection: keep-alive" CRLF) - 1);
> +
> +        if (r->http_version < NGX_HTTP_VERSION_11 || clcf->keepalive_header) {
> +            b->last = ngx_cpymem(b->last, "Connection: keep-alive" CRLF,
> +                                 sizeof("Connection: keep-alive" CRLF) - 1);
> +        }
>
>          if (clcf->keepalive_header) {
>              b->last = ngx_sprintf(b->last, "Keep-Alive: timeout=%T" CRLF,
>
>
> --
> Maxim Dounin
> http://nginx.org/en/donation.html
>
> _______________________________________________
> nginx-ru mailing list
> nginx-ru at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-ru