<div dir="ltr">Hello,<div><br></div><div style>Thanks for your comments.</div><div style>I tried to analyze it and came up with new patch.</div><div style><br></div><div style><br></div><div style><div># HG changeset patch</div>
<div># User <a href="mailto:ykirpichev@gmail.com">ykirpichev@gmail.com</a></div><div># Date 1371110599 -14400</div><div># Branch nopush_fix_3</div><div># Node ID f0a2291a34dae24a78bf3e97d64d2d0f9e37c09e</div><div># Parent 725fb71ab1a60bd48b0afb8b001b5349f5054cb1</div>
<div>SPDY: fix nopush cleanup for SPDY connection</div><div><br></div><div>diff -r 725fb71ab1a6 -r f0a2291a34da src/http/ngx_http_spdy.c</div><div>--- a/src/http/ngx_http_spdy.c Fri Jun 07 13:16:00 2013 -0700</div><div>+++ b/src/http/ngx_http_spdy.c Thu Jun 13 12:03:19 2013 +0400</div>
<div>@@ -378,6 +378,58 @@</div><div><br></div><div> sc->blocked = 0;</div><div><br></div><div>+ // It is better to use NGX_SPDY_WRITE_BUFFERED here, but</div><div>+ // it is defined in ngx_http_spdy_filter_module.c</div>
<div>+ // So, just use !c->buffered</div><div>+ if (!c->buffered && !c->error) {</div><div>+ //no buffered data, so, we should clean nopush if needed</div><div>+ int tcp_nodelay;</div>
<div>+ ngx_http_core_loc_conf_t *clcf;</div><div>+</div><div>+ clcf = ngx_http_get_module_loc_conf(sc->http_connection->conf_ctx,</div><div>+ ngx_http_core_module);</div>
<div>+</div><div>+ if (c->tcp_nopush == NGX_TCP_NOPUSH_SET) {</div><div>+ if (ngx_tcp_push(c->fd) == -1) {</div><div>+ ngx_connection_error(c, ngx_socket_errno, ngx_tcp_push_n " failed");</div>
<div>+ ngx_http_spdy_finalize_connection(sc, NGX_HTTP_INTERNAL_SERVER_ERROR);</div><div>+ return;</div><div>+ }</div><div>+</div><div>+ c->tcp_nopush = NGX_TCP_NOPUSH_UNSET;</div>
<div>+ tcp_nodelay = ngx_tcp_nodelay_and_tcp_nopush ? 1 : 0;</div><div>+</div><div>+ } else {</div><div>+ tcp_nodelay = 1;</div><div>+ }</div><div>+</div><div>+ if (tcp_nodelay</div>
<div>+ && clcf->tcp_nodelay</div><div>+ && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)</div><div>+ {</div><div>+ ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "tcp_nodelay");</div>
<div>+</div><div>+ if (setsockopt(c->fd, IPPROTO_TCP, TCP_NODELAY,</div><div>+ (const void *) &tcp_nodelay, sizeof(int))</div><div>+ == -1)</div><div>+ {</div>
<div>+#if (NGX_SOLARIS)</div><div>+ /* Solaris returns EINVAL if a socket has been shut down */</div><div>+ c->log_error = NGX_ERROR_IGNORE_EINVAL;</div><div>+#endif</div><div>+</div><div>
+ ngx_connection_error(c, ngx_socket_errno,</div><div>+ "setsockopt(TCP_NODELAY) failed");</div><div>+</div><div>+ c->log_error = NGX_ERROR_INFO;</div>
<div>+ ngx_http_spdy_finalize_connection(sc, NGX_HTTP_INTERNAL_SERVER_ERROR);</div><div>+ return;</div><div>+ }</div><div>+</div><div>+ c->tcp_nodelay = NGX_TCP_NODELAY_SET;</div>
<div>+ }</div><div>+ }</div><div>+</div><div> if (sc->processing) {</div><div> if (rev->timer_set) {</div><div> ngx_del_timer(rev);</div><div><br></div><div><br></div><div style>Did I understand you correctly that previous version was incorrect, because even though write is successful, there are might be more incoming data which need to be handled thus there is no sense to clear nopush option, since we might send more data within the same read iteration.</div>
<div style><br></div><div style>BR/ Yury</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/6/11 Yury Kirpichev <span dir="ltr"><<a href="mailto:ykirpichev@gmail.com" target="_blank">ykirpichev@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hello,<div><br></div><div>Thanks for your comments.</div><div>How about second version:</div><div><br></div>
<div><div># HG changeset patch</div><div># User <a href="mailto:ykirpichev@gmail.com" target="_blank">ykirpichev@gmail.com</a></div>
<div># Date 1370964275 -14400</div><div># Branch nopush_fix_2</div><div># Node ID 14de55787a48327019d549d48abf2631e294b4d8</div><div># Parent 725fb71ab1a60bd48b0afb8b001b5349f5054cb1</div><div>SPDY: fix nopush cleanup</div>
<div><br></div><div>diff -r 725fb71ab1a6 -r 14de55787a48 src/http/ngx_http_spdy.c</div><div>--- a/src/http/ngx_http_spdy.c Fri Jun 07 13:16:00 2013 -0700</div><div>+++ b/src/http/ngx_http_spdy.c Tue Jun 11 19:24:35 2013 +0400</div>
<div>@@ -504,6 +504,51 @@</div><div> clcf = ngx_http_get_module_loc_conf(sc->http_connection->conf_ctx,</div><div> ngx_http_core_module);</div><div><br></div><div>+ // all data is sent, can clean nopush if necessary</div>
<div>+ if (wev->ready)</div><div>+ {</div><div>+ int tcp_nodelay;</div><div>+</div><div>+ if (c->tcp_nopush == NGX_TCP_NOPUSH_SET) {</div><div>+ if (ngx_tcp_push(c->fd) == -1) {</div>
<div>+ ngx_connection_error(c, ngx_socket_errno, ngx_tcp_push_n " failed");</div><div>+ return NGX_ERROR;</div><div>+ }</div><div>+</div><div>+ c->tcp_nopush = NGX_TCP_NOPUSH_UNSET;</div>
<div>+ tcp_nodelay = ngx_tcp_nodelay_and_tcp_nopush ? 1 : 0;</div><div>+</div><div>+ } else {</div><div>+ tcp_nodelay = 1;</div><div>+ }</div><div>+</div><div>+ if (tcp_nodelay</div>
<div>+ && clcf->tcp_nodelay</div><div>+ && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)</div><div>+ {</div><div>+ ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "tcp_nodelay");</div>
<div>+</div><div>+ if (setsockopt(c->fd, IPPROTO_TCP, TCP_NODELAY,</div><div>+ (const void *) &tcp_nodelay, sizeof(int))</div><div>+ == -1)</div><div>+ {</div>
<div>+#if (NGX_SOLARIS)</div><div>+ /* Solaris returns EINVAL if a socket has been shut down */</div><div>+ c->log_error = NGX_ERROR_IGNORE_EINVAL;</div><div>+#endif</div><div>+</div><div>
+ ngx_connection_error(c, ngx_socket_errno,</div><div>+ "setsockopt(TCP_NODELAY) failed");</div><div>+</div><div>+ c->log_error = NGX_ERROR_INFO;</div>
<div>+ return NGX_ERROR;</div><div>+ }</div><div>+</div><div>+ c->tcp_nodelay = NGX_TCP_NODELAY_SET;</div><div>+ }</div><div>+</div><div>+ }</div><div>+</div><div> if (ngx_handle_write_event(wev, clcf->send_lowat) != NGX_OK) {</div>
<div> return NGX_ERROR; /* FIXME */</div><div> }</div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/6/11 Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hello!<br>
<div><br>
On Tue, Jun 11, 2013 at 01:00:42PM +0400, Yury Kirpichev wrote:<br>
<br>
> Could you please take a look at patch below.<br>
> I've tried to fix problem that TCP_CORK option is not cleaned in SPDY case.<br>
<br>
</div>The patch looks wrong.<br>
<br>
- It introduces layering violation and build failure --without-http<br>
as a result. You may have better luck focusing on a problem you<br>
want to fix, and avoiding unrelated changes as much as possible.<br>
<br>
- It tries to restore nopush after each c->send_chain() call,<br>
which looks suboptimal. It probably should be done if there are no<br>
pending data to send.<span class="HOEnZb"><font color="#888888"><br>
<span><font color="#888888"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/en/donation.html" target="_blank">http://nginx.org/en/donation.html</a><br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</font></span></font></span></blockquote></div><br></div></div></div>
</blockquote></div><br></div></div>