Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On 12/8/21 7:30 PM, Yann Ylavic wrote: > On Wed, Dec 8, 2021 at 5:19 PM Ruediger Pluem wrote: >> >> Should I take care of adjusting mod_proxy_connect and mod_proxy_http to >> align this again with standard flows and in a second step >> we take care of that r->async_status idea? > > It's fine by me to restore the previous behavior and add > r->async_status if/when really needed. r1895715 Regards Rüdiger
Re: release vibes?
On 12/8/21 13:56, Joe Orton wrote: > On Mon, Dec 06, 2021 at 11:36:51AM +0100, Stefan Eissing wrote: >> Friends of httpd, how do you feel about a release in the next two weeks? > > Sounds good to me. We may have fewer people around to test it, but at > least trying to get a release out is better than definitely having no > release IMO. > +1 let's try to cook a release. Giovanni OpenPGP_signature Description: OpenPGP digital signature
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On Wed, Dec 8, 2021 at 5:19 PM Ruediger Pluem wrote: > > Should I take care of adjusting mod_proxy_connect and mod_proxy_http to align > this again with standard flows and in a second step > we take care of that r->async_status idea? It's fine by me to restore the previous behavior and add r->async_status if/when really needed. Regards; Yann.
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On 12/8/21 2:49 PM, Yann Ylavic wrote:
> On Wed, Dec 8, 2021 at 2:42 PM Ruediger Pluem wrote:
>>
>>
>> On 12/8/21 2:26 PM, Ruediger Pluem wrote:
>>>
>>>
>>> On 11/3/19 4:48 PM, yla...@apache.org wrote:
Author: ylavic
>>
+rc = ap_proxy_tunnel_run(tunnel);
+if (ap_is_HTTP_ERROR(rc)) {
+/* Don't send an error page if we sent data already */
+if (proxyport && !tunnel->replied) {
+return rc;
}
-} while (!done);
-
-ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
- "finished with poll() - cleaning up");
+/* Custom log may need this, still */
+r->status = rc;
>>>
>>> I have some lively discussions outside this forum on the consequences the
>>> above line has.
>>> In fact this means that even though we replied to the client with a 200 on
>>> its CONNECT request we might log a different status
>>> code if something goes wrong during the tunneling. In contrast to this we
>>> don't do this for other other methods e.g. GET or POST:
>>> Once we sent the status code to the client we do not log a different one
>>> should something go wrong while delivering the response.
>>> Removing r->status = rc is IMHO enough to align the behavior again.
>>>
>>> Thoughts?
>>>
>>
>> BTW: We do a similar thing in mod_proxy_http.c in the backend replied with
>> Switching Protocols:
>>
>> status = ap_proxy_tunnel_run(tunnel);
>> if (ap_is_HTTP_ERROR(status)) {
>> /* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
>> * but we can differentiate between client and backend here.
>> */
>> if (status == HTTP_GATEWAY_TIME_OUT
>> && tunnel->timeout == client_timeout) {
>> status = HTTP_REQUEST_TIME_OUT;
>> }
>> }
>> else {
>> /* Update r->status for custom log */
>> status = HTTP_SWITCHING_PROTOCOLS;
>> }
>> r->status = status;
>>
>> Hence we have the same sort of different behavior compared to "ordinary"
>> requests here as well. Hence should we align here as well
>> and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error
>> while tunneling the protocol?
>
> I think you are right, the goal was to show in the access_log how
> tunneling went (primary for the Upgrade case, didn't think too much of
> CONNECT actually :/), but if worthy it looks better to have a new
> r->async_status for that because we are missing the initial status
> now..
Should I take care of adjusting mod_proxy_connect and mod_proxy_http to align
this again with standard flows and in a second step
we take care of that r->async_status idea?
Regards
Rüdiger
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On Wed, Dec 8, 2021 at 2:42 PM Ruediger Pluem wrote:
>
>
> On 12/8/21 2:26 PM, Ruediger Pluem wrote:
> >
> >
> > On 11/3/19 4:48 PM, yla...@apache.org wrote:
> >> Author: ylavic
>
> >>
> >> +rc = ap_proxy_tunnel_run(tunnel);
> >> +if (ap_is_HTTP_ERROR(rc)) {
> >> +/* Don't send an error page if we sent data already */
> >> +if (proxyport && !tunnel->replied) {
> >> +return rc;
> >> }
> >> -} while (!done);
> >> -
> >> -ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> >> - "finished with poll() - cleaning up");
> >> +/* Custom log may need this, still */
> >> +r->status = rc;
> >
> > I have some lively discussions outside this forum on the consequences the
> > above line has.
> > In fact this means that even though we replied to the client with a 200 on
> > its CONNECT request we might log a different status
> > code if something goes wrong during the tunneling. In contrast to this we
> > don't do this for other other methods e.g. GET or POST:
> > Once we sent the status code to the client we do not log a different one
> > should something go wrong while delivering the response.
> > Removing r->status = rc is IMHO enough to align the behavior again.
> >
> > Thoughts?
> >
>
> BTW: We do a similar thing in mod_proxy_http.c in the backend replied with
> Switching Protocols:
>
> status = ap_proxy_tunnel_run(tunnel);
> if (ap_is_HTTP_ERROR(status)) {
> /* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
> * but we can differentiate between client and backend here.
> */
> if (status == HTTP_GATEWAY_TIME_OUT
> && tunnel->timeout == client_timeout) {
> status = HTTP_REQUEST_TIME_OUT;
> }
> }
> else {
> /* Update r->status for custom log */
> status = HTTP_SWITCHING_PROTOCOLS;
> }
> r->status = status;
>
> Hence we have the same sort of different behavior compared to "ordinary"
> requests here as well. Hence should we align here as well
> and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error while
> tunneling the protocol?
I think you are right, the goal was to show in the access_log how
tunneling went (primary for the Upgrade case, didn't think too much of
CONNECT actually :/), but if worthy it looks better to have a new
r->async_status for that because we are missing the initial status
now..
Regards;
Yann.
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On 12/8/21 2:26 PM, Ruediger Pluem wrote:
>
>
> On 11/3/19 4:48 PM, yla...@apache.org wrote:
>> Author: ylavic
>>
>> +rc = ap_proxy_tunnel_run(tunnel);
>> +if (ap_is_HTTP_ERROR(rc)) {
>> +/* Don't send an error page if we sent data already */
>> +if (proxyport && !tunnel->replied) {
>> +return rc;
>> }
>> -} while (!done);
>> -
>> -ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>> - "finished with poll() - cleaning up");
>> +/* Custom log may need this, still */
>> +r->status = rc;
>
> I have some lively discussions outside this forum on the consequences the
> above line has.
> In fact this means that even though we replied to the client with a 200 on
> its CONNECT request we might log a different status
> code if something goes wrong during the tunneling. In contrast to this we
> don't do this for other other methods e.g. GET or POST:
> Once we sent the status code to the client we do not log a different one
> should something go wrong while delivering the response.
> Removing r->status = rc is IMHO enough to align the behavior again.
>
> Thoughts?
>
BTW: We do a similar thing in mod_proxy_http.c in the backend replied with
Switching Protocols:
status = ap_proxy_tunnel_run(tunnel);
if (ap_is_HTTP_ERROR(status)) {
/* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
* but we can differentiate between client and backend here.
*/
if (status == HTTP_GATEWAY_TIME_OUT
&& tunnel->timeout == client_timeout) {
status = HTTP_REQUEST_TIME_OUT;
}
}
else {
/* Update r->status for custom log */
status = HTTP_SWITCHING_PROTOCOLS;
}
r->status = status;
Hence we have the same sort of different behavior compared to "ordinary"
requests here as well. Hence should we align here as well
and always log HTTP_SWITCHING_PROTOCOLS no matter if there was an error while
tunneling the protocol?
Regards
Rüdiger
Re: svn commit: r1869338 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_connect.c modules/proxy/mod_proxy_wstunne
On 11/3/19 4:48 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Sun Nov 3 15:48:53 2019
> New Revision: 1869338
>
> URL: http://svn.apache.org/viewvc?rev=1869338&view=rev
> Log:
> mod_proxy: factorize mod_proxy_{connect,wstunnel} tunneling code in
> proxy_util.
>
> This commit adds struct proxy_tunnel_rec that contains the fields needed for a
> poll() loop through the filters chains, plus functions
> ap_proxy_tunnel_create()
> and ap_proxy_tunnel_run() to respectively initialize a tunnel and (re)start
> it.
>
> Proxy connect and wstunnel modules now make use of this new API to avoid
> duplicating logic and code.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c?rev=1869338&r1=1869337&r2=1869338&view=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c Sun Nov 3 15:48:53
> 2019
> @@ -364,83 +333,25 @@ static int proxy_connect_handler(request
> * Handle two way transfer of data over the socket (this is a tunnel).
> */
>
> -/* we are now acting as a tunnel - the input/output filter stacks should
> - * not contain any non-connection filters.
> - */
> -r->output_filters = c->output_filters;
> -r->proto_output_filters = c->output_filters;
> -r->input_filters = c->input_filters;
> -r->proto_input_filters = c->input_filters;
> -/*r->sent_bodyct = 1;*/
> -
> -do { /* Loop until done (one side closes the connection, or an error) */
> -rv = apr_pollset_poll(pollset, -1, &pollcnt, &signalled);
> -if (rv != APR_SUCCESS) {
> -if (APR_STATUS_IS_EINTR(rv)) {
> -continue;
> -}
> -apr_socket_close(sock);
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01023)
> "error apr_poll()");
> -return HTTP_INTERNAL_SERVER_ERROR;
> -}
> -
> -ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01024)
> - "woke from poll(), i=%d", pollcnt);
> +/* r->sent_bodyct = 1; */
>
> -for (pi = 0; pi < pollcnt; pi++) {
> -const apr_pollfd_t *cur = &signalled[pi];
> -
> -if (cur->desc.s == sock) {
> -pollevent = cur->rtnevents;
> -if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> -ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> APLOGNO(01025)
> - "backend was readable");
> -done |= ap_proxy_transfer_between_connections(r,
> backconn,
> - c, bb_back,
> - bb_front,
> - "backend",
> NULL,
> -
> CONN_BLKSZ, 1)
> - !=
> APR_SUCCESS;
> -}
> -else if (pollevent & APR_POLLERR) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01026)
> - "err on backend connection");
> -backconn->aborted = 1;
> -done = 1;
> -}
> -}
> -else if (cur->desc.s == client_socket) {
> -pollevent = cur->rtnevents;
> -if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> -ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> APLOGNO(01027)
> - "client was readable");
> -done |= ap_proxy_transfer_between_connections(r, c,
> - backconn,
> - bb_front,
> - bb_back,
> - "client",
> - NULL,
> -
> CONN_BLKSZ, 1)
> - !=
> APR_SUCCESS;
> -}
> -else if (pollevent & APR_POLLERR) {
> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02827)
> -
Re: release vibes?
> Am 08.12.2021 um 13:56 schrieb Joe Orton : > > On Mon, Dec 06, 2021 at 11:36:51AM +0100, Stefan Eissing wrote: >> Friends of httpd, how do you feel about a release in the next two weeks? > > Sounds good to me. We may have fewer people around to test it, but at > least trying to get a release out is better than definitely having no > release IMO. Excellent. I propose targeting Tuesday 14th for a candidate. If we go later, it becomes very close to the holidays. But if the time is needed, we can do that as well. I will hold back the current http2 backport, as I need some more people to dare test that from the github on their sites. The mod_tls acceptance I understood as we would like to include that as experimental in the next 2.4.x release. If there are no objections, I will prepare that. Kind Regards, Stefan > > Regards, Joe >
Re: release vibes?
On Mon, Dec 06, 2021 at 11:36:51AM +0100, Stefan Eissing wrote: > Friends of httpd, how do you feel about a release in the next two weeks? Sounds good to me. We may have fewer people around to test it, but at least trying to get a release out is better than definitely having no release IMO. Regards, Joe
