Re: svn commit: r1802077 - /httpd/httpd/branches/2.4.x/STATUS

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1802077 - /httpd/httpd/branches/2.4.x/STATUS

Yann Ylavic
On Sun, Jul 16, 2017 at 6:15 PM,  <[hidden email]> wrote:
> Author: elukey
> Date: Sun Jul 16 16:15:28 2017
> New Revision: 1802077
>
> URL: http://svn.apache.org/viewvc?rev=1802077&view=rev
> Log:
> Propose backport of r1802040
[]
>
> +  *) mod_proxy_fcgi: Add the support for mod_proxy's flushpackets and flushwait params
> +     trunk patch: http://svn.apache.org/r1802040
> +     2.4.x patch: trunk works (modulo CHANGES)
> +     +1: elukey

Not introduced by your change, but about "flushwait" in general: any
reason why a value of zero defaults to PROXY_FLUSH_WAIT (10ms)?

I think it'd be interesting to let flushwait=0 be a real zero timeout
(i.e. non-blocking poll() which,  on some systems, is likely a more
efficient syscall than with the current minimal timeout of 1us).

So how about:
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c    (revision 1802058)
+++ modules/proxy/mod_proxy.c    (working copy)
@@ -277,10 +277,7 @@ static const char *set_worker_param(apr_pool_t *p,
         if (timeout > 1000000 || timeout < 0) {
             return "flushwait must be <= 1s, or 0 for system default
of 10 millseconds.";
         }
-        if (timeout == 0)
-            worker->s->flush_wait = PROXY_FLUSH_WAIT;
-        else
-            worker->s->flush_wait = timeout;
+        worker->s->flush_wait = timeout;
     }
     else if (!strcasecmp(key, "ping")) {
         /* Ping/Pong timeout in given unit (default is second).
?

PROXY_FLUSH_WAIT would still be the default when no "flushwait=" is
used (both with flushpackets=on|auto, since this is what we initialize
flush_wait with by default), but "flushwait=0" would work as
expected/optimally IMHO.

It seems that "flushwait=0" is not documented anyway, although the
real question is: is it used with the current behaviour expected
(which looks unlikely to me)?


Regards,
Yann.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1802077 - /httpd/httpd/branches/2.4.x/STATUS

Luca Toscano
Hi Yann,

2017-07-17 13:49 GMT+02:00 Yann Ylavic <[hidden email]>:
On Sun, Jul 16, 2017 at 6:15 PM,  <[hidden email]> wrote:
> Author: elukey
> Date: Sun Jul 16 16:15:28 2017
> New Revision: 1802077
>
> URL: http://svn.apache.org/viewvc?rev=1802077&view=rev
> Log:
> Propose backport of r1802040
[]
>
> +  *) mod_proxy_fcgi: Add the support for mod_proxy's flushpackets and flushwait params
> +     trunk patch: http://svn.apache.org/r1802040
> +     2.4.x patch: trunk works (modulo CHANGES)
> +     +1: elukey

Not introduced by your change, but about "flushwait" in general: any
reason why a value of zero defaults to PROXY_FLUSH_WAIT (10ms)?

I think it'd be interesting to let flushwait=0 be a real zero timeout
(i.e. non-blocking poll() which,  on some systems, is likely a more
efficient syscall than with the current minimal timeout of 1us).

So how about:
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c    (revision 1802058)
+++ modules/proxy/mod_proxy.c    (working copy)
@@ -277,10 +277,7 @@ static const char *set_worker_param(apr_pool_t *p,
         if (timeout > 1000000 || timeout < 0) {
             return "flushwait must be <= 1s, or 0 for system default
of 10 millseconds.";
         }
-        if (timeout == 0)
-            worker->s->flush_wait = PROXY_FLUSH_WAIT;
-        else
-            worker->s->flush_wait = timeout;
+        worker->s->flush_wait = timeout;
     }
     else if (!strcasecmp(key, "ping")) {
         /* Ping/Pong timeout in given unit (default is second).
?

PROXY_FLUSH_WAIT would still be the default when no "flushwait=" is
used (both with flushpackets=on|auto, since this is what we initialize
flush_wait with by default), but "flushwait=0" would work as
expected/optimally IMHO.
 
Thanks for bringing this up, it looks strange to me too; +1 from my side. 
 
It seems that "flushwait=0" is not documented anyway, although the
real question is: is it used with the current behaviour expected
(which looks unlikely to me)?

I didn't get anything good out of the svn history/commits, but I'd say definitely no (at least from the user/admin's point of view).

Luca 
Loading...