Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/1/20 6:35 PM, [hidden email] wrote:

> Author: ylavic
> Date: Wed Jul  1 16:35:48 2020
> New Revision: 1879401
>
> URL: http://svn.apache.org/viewvc?rev=1879401&view=rev
> Log:
> mod_proxy: improved and reentrant tunneling loop.
>
> modules/proxy/mod_proxy.h:
>     Rename AP_PROXY_TRANSFER_SHOULD_YIELD to AP_PROXY_TRANSFER_YIELD_PENDING
>     and add AP_PROXY_TRANSFER_YIELD_MAX_READS.
>
> modules/proxy/mod_proxy_http.c:
> modules/proxy/mod_proxy_wstunnel.c:
>     Removing of reqtimeout filter is now handled by ap_proxy_tunnel_create().
>
> modules/proxy/proxy_util.c:
>     ap_proxy_transfer_between_connections():
>         Reorganize loop to break out early.
>         When AP_PROXY_TRANSFER_YIELD_PENDING, if !ap_filter_should_yield() we
>         still need to run and check ap_filter_output_pending() since it may
>         release pending data.
>         When AP_PROXY_TRANSFER_YIELD_MAX_READS, stop the loop after too much
>         reads (PROXY_TRANSFER_MAX_READS = 10000) to release the thread and
>         give the caller a chance to schedule the other direction.
>         Don't return APR_INCOMPLETE when it comes from an incomplete body
>         detected by ap_http_filter().
>
>     ap_proxy_tunnel_create():
>         Start with POLLOUT on both directions so that any pending output data
>         is flushed first.
>
>     ap_proxy_tunnel_run():
>         Remove re-init/clear of the pollset for each call so that the function
>         is reentrant.
>         Handle POLLOUT before POLLIN so that we can read in the same pass once
>         all buffered output data are flushed, using ap_filter_input_pending()
>         to drain buffered input data.
>
> This is preparatory patch for async websocket tunneling is mod_proxy_http.
>
> Modified:
>     httpd/httpd/trunk/modules/proxy/mod_proxy.h
>     httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>     httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
>     httpd/httpd/trunk/modules/proxy/proxy_util.c
>

> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1879401&r1=1879400&r2=1879401&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jul  1 16:35:48 2020

> @@ -4410,51 +4470,88 @@ PROXY_DECLARE(int) ap_proxy_tunnel_run(p
>                        "proxy: %s: woken up, %i result(s)", scheme, nresults);
>  
>          for (i = 0; i < nresults; i++) {
> -            const apr_pollfd_t *cur = &results[i];
> -            int revents = cur->rtnevents;
> +            const apr_pollfd_t *pfd = &results[i];
> +            struct proxy_tunnel_conn *tc = pfd->client_data;
> +
> +            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> +                          "proxy: %s: #%i: %s: %hx/%hx", scheme, i,
> +                          tc->name, pfd->rtnevents, tc->pfd->reqevents);
>  
>              /* sanity check */
> -            if (cur->desc.s != client->pfd->desc.s
> -                    && cur->desc.s != origin->pfd->desc.s) {
> +            if (pfd->desc.s != client->pfd->desc.s
> +                    && pfd->desc.s != origin->pfd->desc.s) {
>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222)
>                                "proxy: %s: unknown socket in pollset", scheme);
>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>                  goto cleanup;
>              }
> -
> -            in = cur->client_data;
> -            if (revents & APR_POLLOUT) {
> -                in = in->other;
> -            }
> -            else if (!(revents & (APR_POLLIN | APR_POLLHUP))) {
> +            if (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP | APR_POLLOUT))) {
>                  /* this catches POLLERR/POLLNVAL etc.. */
>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220)
>                                "proxy: %s: polling events error (%x)",
> -                              scheme, revents);
> +                              scheme, pfd->rtnevents);
>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>                  goto cleanup;
>              }
> -            out = in->other;
>  
> -            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> -                          "proxy: %s: #%i: %s/%hx => %s/%hx: %x",
> -                          scheme, i, in->name, in->pfd->reqevents,
> -                          out->name, out->pfd->reqevents, revents);
> +            if (pfd->rtnevents & APR_POLLOUT) {
> +                struct proxy_tunnel_conn *out = tc, *in = tc->other;
> +
> +                ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> +                              "proxy: %s: %s output ready",
> +                              scheme, out->name);
> +
> +                rc = ap_filter_output_pending(out->c);
> +                if (rc == OK) {
> +                    /* Keep polling out (only) */
> +                    continue;
> +                }
> +                if (rc != DECLINED) {
> +                    /* Real failure, bail out */
> +                    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10221)
> +                                  "proxy: %s: %s flushing failed (%i)",
> +                                  scheme, out->name, rc);
> +                    goto cleanup;
> +                }
> +                rc = OK;
> +
> +                /* No more pending data. If the input side is not readable
> +                 * anymore it's time to shutdown for write (this direction
> +                 * is over). Otherwise back to normal business.
> +                 */
> +                del_pollset(pollset, out->pfd, APR_POLLOUT);
> +                if (in->readable) {
> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r,
> +                                  "proxy: %s: %s resume writable",
> +                                  scheme, out->name);
> +                    add_pollset(pollset, in->pfd, APR_POLLIN);
> +                    out->writable = 1;
> +                }
> +                else {
> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
> +                                  "proxy: %s: %s write shutdown",
> +                                  scheme, out->name);
> +                    apr_socket_shutdown(out->pfd->desc.s, 1);
> +                }
> +            }
>  
> -            if (in->readable && (in->drain || !(revents & APR_POLLOUT))) {
> +            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)

Why do we check for APR_POLLHUP here as well? I noticed a case where we request only APR_POLLOUT but get back APR_POLLOUT and
APR_POLLHUB which causes an endless loop.

Regards

Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/10/20 9:55 AM, Ruediger Pluem wrote:

>
>
> On 7/1/20 6:35 PM, [hidden email] wrote:
>> Author: ylavic
>> Date: Wed Jul  1 16:35:48 2020
>> New Revision: 1879401
>>
>> URL: http://svn.apache.org/viewvc?rev=1879401&view=rev
>> Log:
>> mod_proxy: improved and reentrant tunneling loop.
>>
>> modules/proxy/mod_proxy.h:
>>     Rename AP_PROXY_TRANSFER_SHOULD_YIELD to AP_PROXY_TRANSFER_YIELD_PENDING
>>     and add AP_PROXY_TRANSFER_YIELD_MAX_READS.
>>
>> modules/proxy/mod_proxy_http.c:
>> modules/proxy/mod_proxy_wstunnel.c:
>>     Removing of reqtimeout filter is now handled by ap_proxy_tunnel_create().
>>
>> modules/proxy/proxy_util.c:
>>     ap_proxy_transfer_between_connections():
>>         Reorganize loop to break out early.
>>         When AP_PROXY_TRANSFER_YIELD_PENDING, if !ap_filter_should_yield() we
>>         still need to run and check ap_filter_output_pending() since it may
>>         release pending data.
>>         When AP_PROXY_TRANSFER_YIELD_MAX_READS, stop the loop after too much
>>         reads (PROXY_TRANSFER_MAX_READS = 10000) to release the thread and
>>         give the caller a chance to schedule the other direction.
>>         Don't return APR_INCOMPLETE when it comes from an incomplete body
>>         detected by ap_http_filter().
>>
>>     ap_proxy_tunnel_create():
>>         Start with POLLOUT on both directions so that any pending output data
>>         is flushed first.
>>
>>     ap_proxy_tunnel_run():
>>         Remove re-init/clear of the pollset for each call so that the function
>>         is reentrant.
>>         Handle POLLOUT before POLLIN so that we can read in the same pass once
>>         all buffered output data are flushed, using ap_filter_input_pending()
>>         to drain buffered input data.
>>
>> This is preparatory patch for async websocket tunneling is mod_proxy_http.
>>
>> Modified:
>>     httpd/httpd/trunk/modules/proxy/mod_proxy.h
>>     httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>>     httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
>>     httpd/httpd/trunk/modules/proxy/proxy_util.c
>>
>
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1879401&r1=1879400&r2=1879401&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jul  1 16:35:48 2020
>
>> @@ -4410,51 +4470,88 @@ PROXY_DECLARE(int) ap_proxy_tunnel_run(p
>>                        "proxy: %s: woken up, %i result(s)", scheme, nresults);
>>  
>>          for (i = 0; i < nresults; i++) {
>> -            const apr_pollfd_t *cur = &results[i];
>> -            int revents = cur->rtnevents;
>> +            const apr_pollfd_t *pfd = &results[i];
>> +            struct proxy_tunnel_conn *tc = pfd->client_data;
>> +
>> +            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>> +                          "proxy: %s: #%i: %s: %hx/%hx", scheme, i,
>> +                          tc->name, pfd->rtnevents, tc->pfd->reqevents);
>>  
>>              /* sanity check */
>> -            if (cur->desc.s != client->pfd->desc.s
>> -                    && cur->desc.s != origin->pfd->desc.s) {
>> +            if (pfd->desc.s != client->pfd->desc.s
>> +                    && pfd->desc.s != origin->pfd->desc.s) {
>>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222)
>>                                "proxy: %s: unknown socket in pollset", scheme);
>>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>>                  goto cleanup;
>>              }
>> -
>> -            in = cur->client_data;
>> -            if (revents & APR_POLLOUT) {
>> -                in = in->other;
>> -            }
>> -            else if (!(revents & (APR_POLLIN | APR_POLLHUP))) {
>> +            if (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP | APR_POLLOUT))) {
>>                  /* this catches POLLERR/POLLNVAL etc.. */
>>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220)
>>                                "proxy: %s: polling events error (%x)",
>> -                              scheme, revents);
>> +                              scheme, pfd->rtnevents);
>>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>>                  goto cleanup;
>>              }
>> -            out = in->other;
>>  
>> -            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>> -                          "proxy: %s: #%i: %s/%hx => %s/%hx: %x",
>> -                          scheme, i, in->name, in->pfd->reqevents,
>> -                          out->name, out->pfd->reqevents, revents);
>> +            if (pfd->rtnevents & APR_POLLOUT) {
>> +                struct proxy_tunnel_conn *out = tc, *in = tc->other;
>> +
>> +                ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>> +                              "proxy: %s: %s output ready",
>> +                              scheme, out->name);
>> +
>> +                rc = ap_filter_output_pending(out->c);
>> +                if (rc == OK) {
>> +                    /* Keep polling out (only) */
>> +                    continue;
>> +                }
>> +                if (rc != DECLINED) {
>> +                    /* Real failure, bail out */
>> +                    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10221)
>> +                                  "proxy: %s: %s flushing failed (%i)",
>> +                                  scheme, out->name, rc);
>> +                    goto cleanup;
>> +                }
>> +                rc = OK;
>> +
>> +                /* No more pending data. If the input side is not readable
>> +                 * anymore it's time to shutdown for write (this direction
>> +                 * is over). Otherwise back to normal business.
>> +                 */
>> +                del_pollset(pollset, out->pfd, APR_POLLOUT);
>> +                if (in->readable) {
>> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r,
>> +                                  "proxy: %s: %s resume writable",
>> +                                  scheme, out->name);
>> +                    add_pollset(pollset, in->pfd, APR_POLLIN);
>> +                    out->writable = 1;
>> +                }
>> +                else {
>> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
>> +                                  "proxy: %s: %s write shutdown",
>> +                                  scheme, out->name);
>> +                    apr_socket_shutdown(out->pfd->desc.s, 1);
>> +                }
>> +            }
>>  
>> -            if (in->readable && (in->drain || !(revents & APR_POLLOUT))) {
>> +            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
>
> Why do we check for APR_POLLHUP here as well? I noticed a case where we request only APR_POLLOUT but get back APR_POLLOUT and
> APR_POLLHUB which causes an endless loop.

The following patch seems to break the spin:

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1879543)
+++ modules/proxy/proxy_util.c (working copy)
@@ -4527,7 +4527,7 @@
                 }
             }

-            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
+            if (pfd->rtnevents & pfd->reqevents & (APR_POLLIN | APR_POLLHUP)
                     || (tc->readable && tc->other->writable
                         && ap_filter_input_pending(tc->c) == OK)) {
                 struct proxy_tunnel_conn *in = tc, *out = tc->other;


Masking the results first with what we requested breaks the spin.


Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Yann Ylavic
In reply to this post by Ruediger Pluem
On Fri, Jul 10, 2020 at 9:55 AM Ruediger Pluem <[hidden email]> wrote:

>
> On 7/1/20 6:35 PM, [hidden email] wrote:
> > Author: ylavic
> > Date: Wed Jul  1 16:35:48 2020
> > New Revision: 1879401
> >
> >          for (i = 0; i < nresults; i++) {
> > -            const apr_pollfd_t *cur = &results[i];
> > -            int revents = cur->rtnevents;
> > +            const apr_pollfd_t *pfd = &results[i];
> > +            struct proxy_tunnel_conn *tc = pfd->client_data;
> > +
> > +            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> > +                          "proxy: %s: #%i: %s: %hx/%hx", scheme, i,
> > +                          tc->name, pfd->rtnevents, tc->pfd->reqevents);
> >
> >              /* sanity check */
> > -            if (cur->desc.s != client->pfd->desc.s
> > -                    && cur->desc.s != origin->pfd->desc.s) {
> > +            if (pfd->desc.s != client->pfd->desc.s
> > +                    && pfd->desc.s != origin->pfd->desc.s) {
> >                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222)
> >                                "proxy: %s: unknown socket in pollset", scheme);
> >                  rc = HTTP_INTERNAL_SERVER_ERROR;
> >                  goto cleanup;
> >              }
> > -
> > -            in = cur->client_data;
> > -            if (revents & APR_POLLOUT) {
> > -                in = in->other;
> > -            }
> > -            else if (!(revents & (APR_POLLIN | APR_POLLHUP))) {
> > +            if (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP | APR_POLLOUT))) {
> >                  /* this catches POLLERR/POLLNVAL etc.. */
> >                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220)
> >                                "proxy: %s: polling events error (%x)",
> > -                              scheme, revents);
> > +                              scheme, pfd->rtnevents);
> >                  rc = HTTP_INTERNAL_SERVER_ERROR;
> >                  goto cleanup;
> >              }
> > -            out = in->other;
> >
> > -            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> > -                          "proxy: %s: #%i: %s/%hx => %s/%hx: %x",
> > -                          scheme, i, in->name, in->pfd->reqevents,
> > -                          out->name, out->pfd->reqevents, revents);
> > +            if (pfd->rtnevents & APR_POLLOUT) {
> > +                struct proxy_tunnel_conn *out = tc, *in = tc->other;
> > +
> > +                ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
> > +                              "proxy: %s: %s output ready",
> > +                              scheme, out->name);
> > +
> > +                rc = ap_filter_output_pending(out->c);
> > +                if (rc == OK) {
> > +                    /* Keep polling out (only) */
> > +                    continue;
> > +                }
> > +                if (rc != DECLINED) {
> > +                    /* Real failure, bail out */
> > +                    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10221)
> > +                                  "proxy: %s: %s flushing failed (%i)",
> > +                                  scheme, out->name, rc);
> > +                    goto cleanup;
> > +                }
> > +                rc = OK;
> > +
> > +                /* No more pending data. If the input side is not readable
> > +                 * anymore it's time to shutdown for write (this direction
> > +                 * is over). Otherwise back to normal business.
> > +                 */
> > +                del_pollset(pollset, out->pfd, APR_POLLOUT);
> > +                if (in->readable) {
> > +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r,
> > +                                  "proxy: %s: %s resume writable",
> > +                                  scheme, out->name);
> > +                    add_pollset(pollset, in->pfd, APR_POLLIN);
> > +                    out->writable = 1;
> > +                }
> > +                else {
> > +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
> > +                                  "proxy: %s: %s write shutdown",
> > +                                  scheme, out->name);
> > +                    apr_socket_shutdown(out->pfd->desc.s, 1);
> > +                }
> > +            }
> >
> > -            if (in->readable && (in->drain || !(revents & APR_POLLOUT))) {
> > +            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
>
> Why do we check for APR_POLLHUP here as well? I noticed a case where we request only APR_POLLOUT but get back APR_POLLOUT and
> APR_POLLHUB which causes an endless loop.

Hmm, POLLHUP and POLLOUT are mutually exclusive. When requesting for
POLLOUT only, I don't think we can get POLLHUP alone either, because
even if the peer shut down (FIN) the connection we should still be
able to write, and a RST would cause POLLERR (presumably since a
read() would return an error, as opposed to POLLHUP's read() would
return EOF).

Is the loop you are thinking about when we get POLLHUP (alone, so
don't enter "flushing" in POLLOUT code) and then
ap_proxy_transfer_between_connections() does nothing (because of
pending output data)?
As explained above, I don't think it's a possible state, but maybe we
should handle it as POLLERR just in case..

Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/10/20 11:54 AM, Yann Ylavic wrote:

> On Fri, Jul 10, 2020 at 9:55 AM Ruediger Pluem <[hidden email]> wrote:
>>
>> On 7/1/20 6:35 PM, [hidden email] wrote:
>>> Author: ylavic
>>> Date: Wed Jul  1 16:35:48 2020
>>> New Revision: 1879401
>>>
>>>          for (i = 0; i < nresults; i++) {
>>> -            const apr_pollfd_t *cur = &results[i];
>>> -            int revents = cur->rtnevents;
>>> +            const apr_pollfd_t *pfd = &results[i];
>>> +            struct proxy_tunnel_conn *tc = pfd->client_data;
>>> +
>>> +            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>>> +                          "proxy: %s: #%i: %s: %hx/%hx", scheme, i,
>>> +                          tc->name, pfd->rtnevents, tc->pfd->reqevents);
>>>
>>>              /* sanity check */
>>> -            if (cur->desc.s != client->pfd->desc.s
>>> -                    && cur->desc.s != origin->pfd->desc.s) {
>>> +            if (pfd->desc.s != client->pfd->desc.s
>>> +                    && pfd->desc.s != origin->pfd->desc.s) {
>>>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10222)
>>>                                "proxy: %s: unknown socket in pollset", scheme);
>>>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>>>                  goto cleanup;
>>>              }
>>> -
>>> -            in = cur->client_data;
>>> -            if (revents & APR_POLLOUT) {
>>> -                in = in->other;
>>> -            }
>>> -            else if (!(revents & (APR_POLLIN | APR_POLLHUP))) {
>>> +            if (!(pfd->rtnevents & (APR_POLLIN | APR_POLLHUP | APR_POLLOUT))) {
>>>                  /* this catches POLLERR/POLLNVAL etc.. */
>>>                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10220)
>>>                                "proxy: %s: polling events error (%x)",
>>> -                              scheme, revents);
>>> +                              scheme, pfd->rtnevents);
>>>                  rc = HTTP_INTERNAL_SERVER_ERROR;
>>>                  goto cleanup;
>>>              }
>>> -            out = in->other;
>>>
>>> -            ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>>> -                          "proxy: %s: #%i: %s/%hx => %s/%hx: %x",
>>> -                          scheme, i, in->name, in->pfd->reqevents,
>>> -                          out->name, out->pfd->reqevents, revents);
>>> +            if (pfd->rtnevents & APR_POLLOUT) {
>>> +                struct proxy_tunnel_conn *out = tc, *in = tc->other;
>>> +
>>> +                ap_log_rerror(APLOG_MARK, APLOG_TRACE8, 0, r,
>>> +                              "proxy: %s: %s output ready",
>>> +                              scheme, out->name);
>>> +
>>> +                rc = ap_filter_output_pending(out->c);
>>> +                if (rc == OK) {
>>> +                    /* Keep polling out (only) */
>>> +                    continue;
>>> +                }
>>> +                if (rc != DECLINED) {
>>> +                    /* Real failure, bail out */
>>> +                    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10221)
>>> +                                  "proxy: %s: %s flushing failed (%i)",
>>> +                                  scheme, out->name, rc);
>>> +                    goto cleanup;
>>> +                }
>>> +                rc = OK;
>>> +
>>> +                /* No more pending data. If the input side is not readable
>>> +                 * anymore it's time to shutdown for write (this direction
>>> +                 * is over). Otherwise back to normal business.
>>> +                 */
>>> +                del_pollset(pollset, out->pfd, APR_POLLOUT);
>>> +                if (in->readable) {
>>> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r,
>>> +                                  "proxy: %s: %s resume writable",
>>> +                                  scheme, out->name);
>>> +                    add_pollset(pollset, in->pfd, APR_POLLIN);
>>> +                    out->writable = 1;
>>> +                }
>>> +                else {
>>> +                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
>>> +                                  "proxy: %s: %s write shutdown",
>>> +                                  scheme, out->name);
>>> +                    apr_socket_shutdown(out->pfd->desc.s, 1);
>>> +                }
>>> +            }
>>>
>>> -            if (in->readable && (in->drain || !(revents & APR_POLLOUT))) {
>>> +            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
>>
>> Why do we check for APR_POLLHUP here as well? I noticed a case where we request only APR_POLLOUT but get back APR_POLLOUT and
>> APR_POLLHUB which causes an endless loop.
>
> Hmm, POLLHUP and POLLOUT are mutually exclusive. When requesting for
> POLLOUT only, I don't think we can get POLLHUP alone either, because
> even if the peer shut down (FIN) the connection we should still be
> able to write, and a RST would cause POLLERR (presumably since a
> read() would return an error, as opposed to POLLHUP's read() would
> return EOF).
>
> Is the loop you are thinking about when we get POLLHUP (alone, so
> don't enter "flushing" in POLLOUT code) and then
> ap_proxy_transfer_between_connections() does nothing (because of
> pending output data)?

ap_proxy_transfer_between_connections returned EOF in this case.

> As explained above, I don't think it's a possible state, but maybe we
> should handle it as POLLERR just in case..

I observed 36 which 0x24 or POLLHUP | POLLOUT for rtnevents and
52 which is 0x34 or POLLHUP | POLLOUT | POLLERR for rtnevents

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Yann Ylavic
On Fri, Jul 10, 2020 at 12:25 PM Ruediger Pluem <[hidden email]> wrote:
>
> I observed 36 which 0x24 or POLLHUP | POLLOUT for rtnevents and
> 52 which is 0x34 or POLLHUP | POLLOUT | POLLERR for rtnevents

Thanks, so POLLHUP|POLLERR while POSIX says they are mutually exclusive..

I'd rather fix it with the below though:

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1879448)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -4527,9 +4527,11 @@ PROXY_DECLARE(int) ap_proxy_tunnel_run(proxy_tunne
                 }
             }

-            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
-                    || (tc->readable && tc->other->writable
-                        && ap_filter_input_pending(tc->c) == OK)) {
+            if (tc->readable
+                    && (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP
+                                                     | APR_POLLERR)
+                        || (tc->other->writable
+                            && ap_filter_input_pending(tc->c) == OK))) {
                 struct proxy_tunnel_conn *in = tc, *out = tc->other;
                 int sent = 0;

--

The advantage over "pfd->rtnevents & pfd->reqevents & ..." is that it
still enter the read and call the input filters if there is
POLLERR/HUP (but not EOF already), for all the filters to be aware of
the failure at the network level.

Would that work for you?


Regards;
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/10/20 5:05 PM, Yann Ylavic wrote:

> On Fri, Jul 10, 2020 at 12:25 PM Ruediger Pluem <[hidden email]> wrote:
>>
>> I observed 36 which 0x24 or POLLHUP | POLLOUT for rtnevents and
>> 52 which is 0x34 or POLLHUP | POLLOUT | POLLERR for rtnevents
>
> Thanks, so POLLHUP|POLLERR while POSIX says they are mutually exclusive..
>
> I'd rather fix it with the below though:
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c    (revision 1879448)
> +++ modules/proxy/proxy_util.c    (working copy)
> @@ -4527,9 +4527,11 @@ PROXY_DECLARE(int) ap_proxy_tunnel_run(proxy_tunne
>                  }
>              }
>
> -            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
> -                    || (tc->readable && tc->other->writable
> -                        && ap_filter_input_pending(tc->c) == OK)) {
> +            if (tc->readable
> +                    && (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP
> +                                                     | APR_POLLERR)
> +                        || (tc->other->writable
> +                            && ap_filter_input_pending(tc->c) == OK))) {
>                  struct proxy_tunnel_conn *in = tc, *out = tc->other;
>                  int sent = 0;
>
> --
>
> The advantage over "pfd->rtnevents & pfd->reqevents & ..." is that it
> still enter the read and call the input filters if there is
> POLLERR/HUP (but not EOF already), for all the filters to be aware of
> the failure at the network level.

This was the reason why I did not commit anything yet, because I had the feeling that I don't understand everything completely :_)

>
> Would that work for you?

Seems to work. Go ahead.

Regards

Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/10/20 6:15 PM, Ruediger Pluem wrote:

>
>
> On 7/10/20 5:05 PM, Yann Ylavic wrote:
>> On Fri, Jul 10, 2020 at 12:25 PM Ruediger Pluem <[hidden email]> wrote:
>>>
>>> I observed 36 which 0x24 or POLLHUP | POLLOUT for rtnevents and
>>> 52 which is 0x34 or POLLHUP | POLLOUT | POLLERR for rtnevents
>>
>> Thanks, so POLLHUP|POLLERR while POSIX says they are mutually exclusive..
>>
>> I'd rather fix it with the below though:
>>
>> Index: modules/proxy/proxy_util.c
>> ===================================================================
>> --- modules/proxy/proxy_util.c    (revision 1879448)
>> +++ modules/proxy/proxy_util.c    (working copy)
>> @@ -4527,9 +4527,11 @@ PROXY_DECLARE(int) ap_proxy_tunnel_run(proxy_tunne
>>                  }
>>              }
>>
>> -            if (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP)
>> -                    || (tc->readable && tc->other->writable
>> -                        && ap_filter_input_pending(tc->c) == OK)) {
>> +            if (tc->readable
>> +                    && (pfd->rtnevents & (APR_POLLIN | APR_POLLHUP
>> +                                                     | APR_POLLERR)
>> +                        || (tc->other->writable
>> +                            && ap_filter_input_pending(tc->c) == OK))) {
>>                  struct proxy_tunnel_conn *in = tc, *out = tc->other;
>>                  int sent = 0;
>>
>> --
>>
>> The advantage over "pfd->rtnevents & pfd->reqevents & ..." is that it
>> still enter the read and call the input filters if there is
>> POLLERR/HUP (but not EOF already), for all the filters to be aware of
>> the failure at the network level.
>
> This was the reason why I did not commit anything yet, because I had the feeling that I don't understand everything completely :_)
>
>>
>> Would that work for you?
>
> Seems to work. Go ahead.

Will you commit?

Regards

Rüdiger

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Ruediger Pluem


On 7/14/20 9:11 PM, Ruediger Pluem wrote:
>
>
> On 7/10/20 6:15 PM, Ruediger Pluem wrote:
>>
>>
>> On 7/10/20 5:05 PM, Yann Ylavic wrote:

>>
>> This was the reason why I did not commit anything yet, because I had the feeling that I don't understand everything completely :_)
>>
>>>
>>> Would that work for you?
>>
>> Seems to work. Go ahead.
>
> Will you commit?
>

Will you commit?

Regards

Rüdiger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879401 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_http.c mod_proxy_wstunnel.c proxy_util.c

Yann Ylavic
On Wed, Jul 22, 2020 at 9:15 PM Ruediger Pluem <[hidden email]> wrote:
>
> Will you commit?

r1880200, wider change aiming at addressing this issue and anything
POLLERR related.

Regards;
Yann.