The AP_MODE_INIT triggers the handshake nicely. But the protocol switching 
still happens
on the first read. currently looking how to trigger that best.

The h2_task_input is only ever run on "pseudo" connections. But I agree that
it should pass init down the filter chain. 

//Stefan

> Am 12.10.2015 um 10:54 schrieb Yann Ylavic <ylavic....@gmail.com>:
> 
> Also, I wonder if we don't need:
> 
> Index: modules/http2/h2_task_input.c
> ===================================================================
> --- modules/http2/h2_task_input.c    (revision 1707888)
> +++ modules/http2/h2_task_input.c    (working copy)
> @@ -116,7 +116,7 @@ apr_status_t h2_task_input_read(h2_task_input *inp
>     }
> 
>     if (mode == AP_MODE_INIT) {
> -        return APR_SUCCESS;
> +        return ap_get_brigade(f->c->input_filters, bb, mode, block, 
> readbytes);
>     }
> 
>     if (input->bb) {
> --
> 
> We shouldn't since h2_task_input_read() is called at NETWORK level
> (hence after mod_ssl which "eats" AP_MODE_INIT), but it looks more
> correct (as it would be in mod_ssl) since anyway the
> core_input_filter() can handle it.
> 
> 
> On Mon, Oct 12, 2015 at 10:42 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
>> The key here is using AP_MODE_INIT first (where 0 make sense) before
>> AP_MODE_SPECULATIVE.
>> 
>> 
>> On Mon, Oct 12, 2015 at 10:36 AM, Stefan Eissing
>> <stefan.eiss...@greenbytes.de> wrote:
>>> Yeah, I tried a 0 len read. Unfortunately, the core input filter is not 
>>> happy about that:
>>> 
>>> core_filters.c, line 231
>>>        AP_DEBUG_ASSERT(readbytes > 0);
>>> 
>>>> Am 12.10.2015 um 10:25 schrieb Plüm, Rüdiger, Vodafone Group 
>>>> <ruediger.pl...@vodafone.com>:
>>>> 
>>>> How about the following patch?
>>>> 
>>>> This separates the triggering of the SSL Handshake and the possible 
>>>> reading of the 24 bytes:
>>>> 
>>>> Index: h2_h2.c
>>>> ===================================================================
>>>> --- h2_h2.c     (revision 1707437)
>>>> +++ h2_h2.c     (working copy)
>>>> @@ -154,7 +154,7 @@
>>>> 
>>>>        temp = apr_brigade_create(c->pool, c->bucket_alloc);
>>>>        status = ap_get_brigade(c->input_filters, temp,
>>>> -                                AP_MODE_SPECULATIVE, APR_BLOCK_READ, 24);
>>>> +                                AP_MODE_INIT, APR_BLOCK_READ, 0);
>>>> 
>>>>        if (status == APR_SUCCESS) {
>>>>            if (h2_ctx_protocol_get(c)
>>>> @@ -171,23 +171,31 @@
>>>>                    char *s = NULL;
>>>>                    apr_size_t slen;
>>>> 
>>>> -                    apr_brigade_pflatten(temp, &s, &slen, c->pool);
>>>> -                    if ((slen >= 24) && !memcmp(H2_MAGIC_TOKEN, s, 24)) {
>>>> -                        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
>>>> -                                      "h2_h2, direct mode detected");
>>>> -                        h2_ctx_protocol_set(ctx, is_tls? "h2" : "h2c");
>>>> +                    status = ap_get_brigade(c->input_filters, temp,
>>>> +                                            AP_MODE_SPECULATIVE, 
>>>> APR_BLOCK_READ, 24);
>>>> +                    if (status == APR_SUCCESS) {
>>>> +                        apr_brigade_pflatten(temp, &s, &slen, c->pool);
>>>> +                        if ((slen >= 24) && !memcmp(H2_MAGIC_TOKEN, s, 
>>>> 24)) {
>>>> +                            ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
>>>> +                                          "h2_h2, direct mode detected");
>>>> +                            h2_ctx_protocol_set(ctx, is_tls? "h2" : 
>>>> "h2c");
>>>> +                        }
>>>> +                        else {
>>>> +                            ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
>>>> +                                          "h2_h2, not detected in %d 
>>>> bytes: %s",
>>>> +                                          (int)slen, s);
>>>> +                        }
>>>>                    }
>>>> -                    else {
>>>> -                        ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
>>>> -                                      "h2_h2, not detected in %d bytes: 
>>>> %s",
>>>> -                                      (int)slen, s);
>>>> -                    }
>>>>                }
>>>> +                else {
>>>> +                    ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c,
>>>> +                                  "h2_h2, error reading 24 bytes 
>>>> speculative");
>>>> +                }
>>>>            }
>>>>        }
>>>>        else {
>>>>            ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c,
>>>> -                          "h2_h2, error reading 24 bytes speculative");
>>>> +                          "h2_h2, Failed to init connection");
>>>>        }
>>>>        apr_brigade_destroy(temp);
>>>>    }
>>>> 
>>>> This would still block in the non ssl case if directmode is not set to off 
>>>> explicitly. I would propose to change the default behaviour of directmode 
>>>> here to off as directmode seems to be something very special to me that 
>>>> should be explicitly enabled.
>>>> 
>>>> Regards
>>>> 
>>>> Rüdiger
>>>> 
>>>>> -----Original Message-----
>>>>> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
>>>>> Sent: Sonntag, 11. Oktober 2015 19:54
>>>>> To: dev@httpd.apache.org
>>>>> Subject: Re: 2.4.17 test failure for mod_nntp_like_ssl when mod_http2 is
>>>>> loaded
>>>>> 
>>>>> Ok, in ssl_engine_io.c, lines 1426+ I see a hint:
>>>>> 
>>>>>   /* XXX: we could actually move ssl_io_filter_handshake to an
>>>>>    * ap_hook_process_connection but would still need to call it for
>>>>>    * AP_MODE_INIT for protocols that may upgrade the connection
>>>>>    * rather than have SSLEngine On configured.
>>>>>    */
>>>>>   if ((status = ssl_io_filter_handshake(inctx->filter_ctx)) !=
>>>>> APR_SUCCESS) {
>>>>>       return ssl_io_filter_error(f, bb, status);
>>>>>   }
>>>>> 
>>>>> The handshake handling gets triggered on a filter, so it all happens on
>>>>> READs (or WRITEs), which explains why connection hooks do not see it. This
>>>>> is quite some code which looks like we do not want to touch for a "quick
>>>>> fix". Also, there is no quick fix inside mod_http2 as before handshake,
>>>>> the SNI might also not be there yet, thus the vhost cannot be determined.
>>>>> etc.
>>>>> 
>>>>> For the current release, I think we should leave it as it is and advise
>>>>> that current mod_http2 is incompatible with things like nntp.
>>>>> 
>>>>> For the next release, I'd like the server to perform the handshake at a
>>>>> defined time, so other modules can rely on protocols being selected and
>>>>> correct vhost being known before the first read/write.
>>>>> 
>>>>> //Stefan
>>>>> 
>>> 

Reply via email to