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

[Maxim Dounin <mdounin@xxxxxxxxxx>]

Hello!

On Tue, May 14, 2013 at 10:24:31PM +0600, Илья Шипицин wrote:

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

Я очень не хочу это трогать, по крайней мере - без тщательного  
тестирования.

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

В рассылках периодически жалуются даже на те очень нечастые 
ситуцаии, когда nginx так всё-таки делает (e.g., при 
переконфигурации).

> 
> 14 мая 2013 г., 22:17 пользователь Maxim Dounin <mdounin@xxxxxxxxxx> написал:
> > Hello!
> >
> > On Tue, May 14, 2013 at 09:39:42PM +0600, Илья Шипицин wrote:
> >
> >> 14 мая 2013 г., 21:16 пользователь Maxim Dounin <mdounin@xxxxxxxxxx> 
> >> написал:
> >> > 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@xxxxxxxxx
> > http://mailman.nginx.org/mailman/listinfo/nginx-ru
> _______________________________________________
> nginx-ru mailing list
> nginx-ru@xxxxxxxxx
> http://mailman.nginx.org/mailman/listinfo/nginx-ru

-- 
Maxim Dounin
http://nginx.org/en/donation.html

_______________________________________________
nginx-ru mailing list
nginx-ru@xxxxxxxxx
http://mailman.nginx.org/mailman/listinfo/nginx-ru