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 >>>>> >>>