[PATCH/v3] SPDY: fix nopush cleanup
Yury Kirpichev
ykirpichev at gmail.com
Thu Jun 13 08:12:18 UTC 2013
Hello,
Thanks for your comments.
I tried to analyze it and came up with new patch.
# HG changeset patch
# User ykirpichev at gmail.com
# Date 1371110599 -14400
# Branch nopush_fix_3
# Node ID f0a2291a34dae24a78bf3e97d64d2d0f9e37c09e
# Parent 725fb71ab1a60bd48b0afb8b001b5349f5054cb1
SPDY: fix nopush cleanup for SPDY connection
diff -r 725fb71ab1a6 -r f0a2291a34da src/http/ngx_http_spdy.c
--- a/src/http/ngx_http_spdy.c Fri Jun 07 13:16:00 2013 -0700
+++ b/src/http/ngx_http_spdy.c Thu Jun 13 12:03:19 2013 +0400
@@ -378,6 +378,58 @@
sc->blocked = 0;
+ // It is better to use NGX_SPDY_WRITE_BUFFERED here, but
+ // it is defined in ngx_http_spdy_filter_module.c
+ // So, just use !c->buffered
+ if (!c->buffered && !c->error) {
+ //no buffered data, so, we should clean nopush if needed
+ int tcp_nodelay;
+ ngx_http_core_loc_conf_t *clcf;
+
+ clcf = ngx_http_get_module_loc_conf(sc->http_connection->conf_ctx,
+ ngx_http_core_module);
+
+ if (c->tcp_nopush == NGX_TCP_NOPUSH_SET) {
+ if (ngx_tcp_push(c->fd) == -1) {
+ ngx_connection_error(c, ngx_socket_errno, ngx_tcp_push_n "
failed");
+ ngx_http_spdy_finalize_connection(sc,
NGX_HTTP_INTERNAL_SERVER_ERROR);
+ return;
+ }
+
+ c->tcp_nopush = NGX_TCP_NOPUSH_UNSET;
+ tcp_nodelay = ngx_tcp_nodelay_and_tcp_nopush ? 1 : 0;
+
+ } else {
+ tcp_nodelay = 1;
+ }
+
+ if (tcp_nodelay
+ && clcf->tcp_nodelay
+ && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)
+ {
+ ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "tcp_nodelay");
+
+ if (setsockopt(c->fd, IPPROTO_TCP, TCP_NODELAY,
+ (const void *) &tcp_nodelay, sizeof(int))
+ == -1)
+ {
+#if (NGX_SOLARIS)
+ /* Solaris returns EINVAL if a socket has been shut down */
+ c->log_error = NGX_ERROR_IGNORE_EINVAL;
+#endif
+
+ ngx_connection_error(c, ngx_socket_errno,
+ "setsockopt(TCP_NODELAY) failed");
+
+ c->log_error = NGX_ERROR_INFO;
+ ngx_http_spdy_finalize_connection(sc,
NGX_HTTP_INTERNAL_SERVER_ERROR);
+ return;
+ }
+
+ c->tcp_nodelay = NGX_TCP_NODELAY_SET;
+ }
+ }
+
if (sc->processing) {
if (rev->timer_set) {
ngx_del_timer(rev);
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.
BR/ Yury
2013/6/11 Yury Kirpichev <ykirpichev at gmail.com>
> Hello,
>
> Thanks for your comments.
> How about second version:
>
> # HG changeset patch
> # User ykirpichev at gmail.com
> # Date 1370964275 -14400
> # Branch nopush_fix_2
> # Node ID 14de55787a48327019d549d48abf2631e294b4d8
> # Parent 725fb71ab1a60bd48b0afb8b001b5349f5054cb1
> SPDY: fix nopush cleanup
>
> diff -r 725fb71ab1a6 -r 14de55787a48 src/http/ngx_http_spdy.c
> --- a/src/http/ngx_http_spdy.c Fri Jun 07 13:16:00 2013 -0700
> +++ b/src/http/ngx_http_spdy.c Tue Jun 11 19:24:35 2013 +0400
> @@ -504,6 +504,51 @@
> clcf = ngx_http_get_module_loc_conf(sc->http_connection->conf_ctx,
> ngx_http_core_module);
>
> + // all data is sent, can clean nopush if necessary
> + if (wev->ready)
> + {
> + int tcp_nodelay;
> +
> + if (c->tcp_nopush == NGX_TCP_NOPUSH_SET) {
> + if (ngx_tcp_push(c->fd) == -1) {
> + ngx_connection_error(c, ngx_socket_errno, ngx_tcp_push_n
> " failed");
> + return NGX_ERROR;
> + }
> +
> + c->tcp_nopush = NGX_TCP_NOPUSH_UNSET;
> + tcp_nodelay = ngx_tcp_nodelay_and_tcp_nopush ? 1 : 0;
> +
> + } else {
> + tcp_nodelay = 1;
> + }
> +
> + if (tcp_nodelay
> + && clcf->tcp_nodelay
> + && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)
> + {
> + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "tcp_nodelay");
> +
> + if (setsockopt(c->fd, IPPROTO_TCP, TCP_NODELAY,
> + (const void *) &tcp_nodelay, sizeof(int))
> + == -1)
> + {
> +#if (NGX_SOLARIS)
> + /* Solaris returns EINVAL if a socket has been shut down
> */
> + c->log_error = NGX_ERROR_IGNORE_EINVAL;
> +#endif
> +
> + ngx_connection_error(c, ngx_socket_errno,
> + "setsockopt(TCP_NODELAY) failed");
> +
> + c->log_error = NGX_ERROR_INFO;
> + return NGX_ERROR;
> + }
> +
> + c->tcp_nodelay = NGX_TCP_NODELAY_SET;
> + }
> +
> + }
> +
> if (ngx_handle_write_event(wev, clcf->send_lowat) != NGX_OK) {
> return NGX_ERROR; /* FIXME */
> }
>
>
> 2013/6/11 Maxim Dounin <mdounin at mdounin.ru>
>
>> Hello!
>>
>> On Tue, Jun 11, 2013 at 01:00:42PM +0400, Yury Kirpichev wrote:
>>
>> > Could you please take a look at patch below.
>> > I've tried to fix problem that TCP_CORK option is not cleaned in SPDY
>> case.
>>
>> The patch looks wrong.
>>
>> - It introduces layering violation and build failure --without-http
>> as a result. You may have better luck focusing on a problem you
>> want to fix, and avoiding unrelated changes as much as possible.
>>
>> - It tries to restore nopush after each c->send_chain() call,
>> which looks suboptimal. It probably should be done if there are no
>> pending data to send.
>>
>> --
>> Maxim Dounin
>> http://nginx.org/en/donation.html
>>
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel at nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20130613/ec9552fb/attachment.html>
More information about the nginx-devel
mailing list