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