Ok, got a fix in r1853967 and propose to include in mod_reqtimeout backport.

Cheers, Stefan

> Am 20.02.2019 um 16:09 schrieb ste...@eissing.org:
> 
> Ok, I think I understood:
> 
> - On a h2 slave connection, slave->keepalives is always > 0, to keep certain 
> parts of our server from thinking "Oh, this is the first request!"
> - mod_reqtimeout interprets this as "we are inbetween requests", 
> ccfg->in_keep_alive != 0
> - mod_reqtimeout.c:184+ input filter does a speculative read and expects to 
> see either an error or some data
> - if not, it return the spec read status
> - Here, in the failure case, the base slave_in filter return APR_EAGAIN
>  * this was a fix to prevent ap_check_pipeline() from closing the connection 
> when seeing APR_SUCCESS with 0 bytes
> - The read itself is, in the test case, done by mod_cgid which on != 
> APR_SUCCESS fails with a 400 response
> 
> The patch did not really change this behaviour, but it changed the 
> initialization order. 
> 
> Why does this only trigger in this new test case?
> - Sending requests via mod_proxy_http2 changes the timings of EOF detection. 
> When the EOF is not signalled on the header frame, the request body is 
> indeterminate until a DATA frame with EOF arrives. That may happen after 
> mod_reqtimeout checks.
> - the slave input filter gets called with a SPEC read of 1 byte, has no data 
> and also has not seen an EOF. It returns ARP_EAGAIN.
> 
> What to do?
> - I think the mod_reqtimeout patch is mostly correct and it should be be 
> active on h2 slave connections as well
> - But maybe mod_reqtimeout needs slightly different behaviour on a slave 
> connection. maybe the keepalives spec reads need to be abandoned here.
> - Changing the return code of the h2 slave input filter to return APR_SUCCESS 
> on the speculative read will not really help. mod_reqtimeout will return this 
> to the caller as there was no data. This means that mod_cgid will just call 
> it again in a infinite loop until either data or an EOF arrives. That does 
> not seem good either.
> 
> Any other ideas?
> 
> -Stefan
> 
>> Am 20.02.2019 um 15:14 schrieb ste...@eissing.org:
>> 
>> 
>> 
>>> Am 20.02.2019 um 10:53 schrieb yla...@apache.org:
>>> 
>>> Author: ylavic
>>> Date: Wed Feb 20 09:53:30 2019
>>> New Revision: 1853939
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1853939&view=rev
>>> Log:
>>> Propose.
>>> 
>>> Modified:
>>>  httpd/httpd/branches/2.4.x/STATUS
>>> 
>>> Modified: httpd/httpd/branches/2.4.x/STATUS
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1853939&r1=1853938&r2=1853939&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/branches/2.4.x/STATUS (original)
>>> +++ httpd/httpd/branches/2.4.x/STATUS Wed Feb 20 09:53:30 2019
>>> @@ -246,6 +246,23 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
>>>    2.4.x patch: 
>>> http://people.apache.org/~ylavic/patches/httpd-2.4.x-forward_100_continue-v3.patch
>>>    +1: ylavic
>>> 
>>> +  *) mod_reqtimeout: Allow to configure (TLS-)handshake timeouts.  PR 
>>> 61310.
>>> +     trunk patch: http://svn.apache.org/r1853901
>>> +                  http://svn.apache.org/r1853906
>>> +                  http://svn.apache.org/r1853908
>>> +                  http://svn.apache.org/r1853929
>>> +                  http://svn.apache.org/r1853935
>>> +     2.4.x patch: 
>>> http://people.apache.org/~ylavic/patches/httpd-2.4.x-reqtimeout_handshake.patch
>>> +     +1: ylavic
>> 
>> This one is giving me headaches. I added test_600_01 to master at 
>> https://github.com/icing/mod_h2 that triggers it.
>> 
>> My suspicion is that mod_reqtimeout and mod_http2 need some more 
>> coordination. While the handshake timeout is very useful also for h2 
>> connections, waiting for GETLINE or such is less useful. 
>> 
>> There is more than one path how a connection can go from h1 to h2 (alpn, 
>> direct 24 bytes detection, h1 upgrade:). Direct h2 use by curl/nghttp works, 
>> but mod_proxy_http2 does not.
>> 
>> We have a hook for protocol switching: ap_hook_protocol_switch(...) and 
>> probably mod_reqtimeout needs to listen to that and changes its (at least 
>> part of the) input/output filtering accordingly.
>> 
>> I will try to understand how things go sideways with the change and report 
>> back here.
>> 
>> -Stefan
> 

Reply via email to