Re: Detecting client aborts and stream resets

[Michael Kaufmann]

Zitat von William A Rowe Jr :


On Tue, May 3, 2016 at 9:31 AM, Michael Kaufmann 
wrote:


Hi all,

a content generator module can detect client aborts and stream resets
while it reads the request body. But how can it detect this afterwards,
while the response is being generated?

This is important for HTTP/2, because the client may reset a stream, and
mod_http2 needs to wait for the content generator to finish. Therefore the
content generator should stop generating the response when it is no longer
needed.

Is there any API for this? The "conn_rec->aborted" flag exists, but which
Apache function sets this flag?

If there is no API, maybe an optional function for mod_http2 would be a
solution.



Nope - an optional function in mod_http2 is too special case, generators
must remain protocol (socket or other transport) agnostic.



Sure, official Apache modules should not call protocol-dependent  
hooks, but it could be a solution for 3rd party modules.



In the case of mod_cache'd content, the generator can't quit, it is already
populating a cache, which means you'll generate an invalid cached object.

But if you knew that the cache module wasn't collecting info, r->c->aborted
tells you if anyone is still listening, right?


Unfortunately not. While the content generator is running (just  
preparing the response, it is not reading from the input filters  
anymore and not writing to the output filters yet), Apache does not  
check the connection's state.


I'm not sure how mod_proxy deals with this - does it abort the backend  
request when the client closes the connection?

Re: Detecting client aborts and stream resets

[Mike Rumph]

Hello Michael.

On 5/3/2016 7:31 AM, Michael Kaufmann wrote:

Hi all,

a content generator module can detect client aborts and stream resets 
while it reads the request body. But how can it detect this 
afterwards, while the response is being generated?


This is important for HTTP/2, because the client may reset a stream, 
and mod_http2 needs to wait for the content generator to finish. 
Therefore the content generator should stop generating the response 
when it is no longer needed.


Is there any API for this? The "conn_rec->aborted" flag exists, but 
which Apache function sets this flag?

The conn_rec->aborted flag is set in the following functions:
- ap_core_output_filter() in server/core_filters.c
- ap_process_connection() in server/connection.c
- process_socket() in server/mpm/event/event.c
- worker_main() in server/mpm/winnt/child.c
If there is no API, maybe an optional function for mod_http2 would be 
a solution.


Regards,
Michael

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[Ruediger Pluem]

On 05/03/2016 05:51 PM, William A Rowe Jr wrote:
> On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem  > wrote:
> 
> 
> On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> >
> > On 04/27/2016 08:41 PM, wr...@apache.org  
> wrote:
> >> Author: wrowe
> >> Date: Wed Apr 27 18:41:49 2016
> >> New Revision: 1741310
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1741310&view=rev
> >> Log:
> >>
> >>   Ensure the useragent_ip is only used in the case
> >>   where it has been initialized, fall back on the
> >>   connection's remote_ip if the status is accidently
> >>   updated from an uninitialized request_rec.
> >>
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> >> @@ -501,7 +501,7 @@ static int update_child_status_internal(
> >>  copy_request(ws->request, sizeof(ws->request), r);
> >>  }
> >>
> >> -if (r) {
> >> +if (r && r->useragent_ip) {
> >>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP, 
> NULL)))
> >>  apr_cpystrn(ws->client, r->useragent_ip, 
> sizeof(ws->client));
> >
> > Hm, wouldn't it be better to just encapsulate the above line in an if 
> (r->useragent_ip)
> > or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP, 
> NULL) cannot deliver
> > something meaningful if r->useragent_ip == NULL?
> 
> 
> It *could* deliver; but if r->useragent_ip is unset, this is 
> a premature call to set up the score entry, the request
> read hook hasn't completed and will fail to deliver.  But
> I don't know what you mean by "the above line" - you
> mean the apr_cpystrn?  As you point out, the call is
> fairly useless without a completed request rec and 
> known useragent_ip in the first place (you want to
> look up the hostname of null address - but then 
> guard against copying a NULL string once it has
> failed ? :-)
> 
> By falling back on the conn-based lookup, in the code you 
> snipped... we are populating the conn-based hostname, 
> which may be useful datum elsewhere...

Good point. I missed the else if (c) stuff below. Thanks for pointing out.

Regards

Rüdiger

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Tue, May 3, 2016 at 10:51 AM, William A Rowe Jr 
wrote:

>
> If we want to be more robust, have a look at the current 2.4.x
> implementation of ap_get_useragent_host() in core.c...
>
> if (r->useragent_addr == conn->client_addr) {
> return ap_get_remote_host(conn, r->per_dir_config, type,
> str_is_ip);
> }
>
> Perhaps this would be better...
>
> if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) {
> return ap_get_remote_host(conn, r->per_dir_config, type,
> str_is_ip);
> }
>

Committed and proposed for backport.

Re: svn commit: r1741310 - in /httpd/httpd/trunk: modules/http2/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/ server/mpm/winnt/ server/mpm/worker/

[William A Rowe Jr]

On Mon, May 2, 2016 at 3:09 AM, Ruediger Pluem  wrote:

>
> On 04/28/2016 10:19 AM, Ruediger Pluem wrote:
> >
> > On 04/27/2016 08:41 PM, wr...@apache.org wrote:
> >> Author: wrowe
> >> Date: Wed Apr 27 18:41:49 2016
> >> New Revision: 1741310
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1741310&view=rev
> >> Log:
> >>
> >>   Ensure the useragent_ip is only used in the case
> >>   where it has been initialized, fall back on the
> >>   connection's remote_ip if the status is accidently
> >>   updated from an uninitialized request_rec.
> >>
> >> --- httpd/httpd/trunk/server/scoreboard.c (original)
> >> +++ httpd/httpd/trunk/server/scoreboard.c Wed Apr 27 18:41:49 2016
> >> @@ -501,7 +501,7 @@ static int update_child_status_internal(
> >>  copy_request(ws->request, sizeof(ws->request), r);
> >>  }
> >>
> >> -if (r) {
> >> +if (r && r->useragent_ip) {
> >>  if (!(val = ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL)))
> >>  apr_cpystrn(ws->client, r->useragent_ip,
> sizeof(ws->client));
> >
> > Hm, wouldn't it be better to just encapsulate the above line in an if
> (r->useragent_ip)
> > or do we already know that ap_get_useragent_host(r, REMOTE_NOLOOKUP,
> NULL) cannot deliver
> > something meaningful if r->useragent_ip == NULL?
>

It *could* deliver; but if r->useragent_ip is unset, this is
a premature call to set up the score entry, the request
read hook hasn't completed and will fail to deliver.  But
I don't know what you mean by "the above line" - you
mean the apr_cpystrn?  As you point out, the call is
fairly useless without a completed request rec and
known useragent_ip in the first place (you want to
look up the hostname of null address - but then
guard against copying a NULL string once it has
failed ? :-)

By falling back on the conn-based lookup, in the code you
snipped... we are populating the conn-based hostname,
which may be useful datum elsewhere...

[...]
else if (c) {
if (!(val = ap_get_remote_host(c,
c->base_server->lookup_defaults,
   REMOTE_NOLOOKUP, NULL)))
apr_cpystrn(ws->client, c->client_ip, sizeof(ws->client));
else
apr_cpystrn(ws->client, val, sizeof(ws->client));

In all cases we should have the value for (c) where (r) is passed,
whether r was fully populated yet or not.

If we want to be more robust, have a look at the current 2.4.x
implementation of ap_get_useragent_host() in core.c...

if (r->useragent_addr == conn->client_addr) {
return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
}

Perhaps this would be better...

if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) {
return ap_get_remote_host(conn, r->per_dir_config, type, str_is_ip);
}

Re: [PATCH 54255] mod_deflate adjusts the headers "too late"

[William A Rowe Jr]

Keep in mind that such changes alter the expected behavior that all of the
third party module authors have worked around.  This doesn't seem like
a suitable change for 2.4.x maintenance branch but a good improvement
for trunk, looking forwards.

On Tue, May 3, 2016 at 9:42 AM, Michael Kaufmann 
wrote:

> Hi,
>
> It would be great if somebody could have a look at the proposed patch.
>
> The problem: Request headers are adjusted too late (in the input filter).
> Proposed solution: Adjust the request headers in the filter init function.
>
> https://bz.apache.org/bugzilla/show_bug.cgi?id=54255
>
> Regards,
> Michael
>
>

Re: Detecting client aborts and stream resets

[William A Rowe Jr]

On Tue, May 3, 2016 at 9:31 AM, Michael Kaufmann 
wrote:

> Hi all,
>
> a content generator module can detect client aborts and stream resets
> while it reads the request body. But how can it detect this afterwards,
> while the response is being generated?
>
> This is important for HTTP/2, because the client may reset a stream, and
> mod_http2 needs to wait for the content generator to finish. Therefore the
> content generator should stop generating the response when it is no longer
> needed.
>
> Is there any API for this? The "conn_rec->aborted" flag exists, but which
> Apache function sets this flag?
>
> If there is no API, maybe an optional function for mod_http2 would be a
> solution.
>

Nope - an optional function in mod_http2 is too special case, generators
must remain protocol (socket or other transport) agnostic.

In the case of mod_cache'd content, the generator can't quit, it is already
populating a cache, which means you'll generate an invalid cached object.

But if you knew that the cache module wasn't collecting info, r->c->aborted
tells you if anyone is still listening, right?

[PATCH 54255] mod_deflate adjusts the headers "too late"

[Michael Kaufmann]

Hi,

It would be great if somebody could have a look at the proposed patch.

The problem: Request headers are adjusted too late (in the input filter).
Proposed solution: Adjust the request headers in the filter init function.

https://bz.apache.org/bugzilla/show_bug.cgi?id=54255

Regards,
Michael

Detecting client aborts and stream resets

[Michael Kaufmann]

Hi all,

a content generator module can detect client aborts and stream resets  
while it reads the request body. But how can it detect this  
afterwards, while the response is being generated?


This is important for HTTP/2, because the client may reset a stream,  
and mod_http2 needs to wait for the content generator to finish.  
Therefore the content generator should stop generating the response  
when it is no longer needed.


Is there any API for this? The "conn_rec->aborted" flag exists, but  
which Apache function sets this flag?


If there is no API, maybe an optional function for mod_http2 would be  
a solution.


Regards,
Michael

Re: svn commit: r1734238 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_http2.xml modules/http2/h2_proxy_session.c modules/http2/mod_proxy_http2.c

[Stefan Eissing]

Thanks, Eric. Fixed in r1742073.

-Stefan

> Am 02.05.2016 um 20:25 schrieb Eric Covener :
> 
> On Wed, Mar 9, 2016 at 8:41 AM,   wrote:
>> +mod_proxy_http2 works with incoming requests
>> +over HTTP/1.1 and HTTP/2 requests. If mod_proxy_http2
>> +handles the frontend connection, requests against the same HTTP/2
>> +backend are sent over a single connection, whenever possible.
> 
> 
> Should the second module be mod_http2 here?