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

[Maxim Dounin]

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