async keepalive in http/2

[Stefan Eissing <st...@greenbytes.de>]

With the timeout behaviour of SSL reads fixed, I am making another attempt at have http/2 connections behave properly in async MPMs, e.g. event. One thing for that necessary as change in server is the hook to send a last GOAWAY frame before the connection is closed. Not sending it seems to confuse browsers, leading to unwanted failure of the subsequent request.

You opinion therefore is required. On inspection of the connection shutdown handling, it seems that *all* MPM modules call, directly or indirectly, ap_start_lingering_close(). Keepalive connections are still healthy at this point and pool cleanup happens later, so this seems to be the place to install a new hook:

AP_DECLARE_HOOK(int,pre_close_connection,(conn_rec *c)) as RUN_ALL

Does this seem like the right place? Is the name any good? 

(forget the question about naming, should not have disturbed the beast...)

Cheers,

  Stefan

Re: async keepalive in http/2

[Stefan Eissing <st...@greenbytes.de>]

First experiments show that event does not always call ap_start_lingering_close(c) and exactly for the interesting cases.

As I read it, the reason is that ap_start_lingering_close() does a flush on the connection and event wants to start lingering close when shifting the connection from its keepalive queue to the proper lingering queue. It is not holding a lock, but its a tight loop where it does not like to block.

So, event has its own variants of start_lingering_close. I added 

ap_prep_lingering_close(c)

to the API which is then called from everyone and runs the pre_close hook. With that in place, I see proper GOAWAYs in Chrome on a keepalive connection. 

However if a pre_close hook wants to do more, or if we do care about protocols that need a real conversation on a keep alive shutdown, we need some mechanism to let the protocol indicate, maybe as return from pre_close (making it run_first) that the connection should be transferred back from
keepalive to normal processing. It would then be up to the protocol handler to manage the state and all...

More complicated and probably overkill for the <32 bytes http/2 likes to send. I just wanted to mention it...

Cheers,

  Stefan



> Am 22.01.2016 um 16:13 schrieb Jim Jagielski <ji...@jaguNET.com>:
> 
> +1
> 
>> On Jan 22, 2016, at 9:17 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>> 
>> 
>>> Am 22.01.2016 um 15:16 schrieb Eric Covener <co...@gmail.com>:
>>> 
>>> On Fri, Jan 22, 2016 at 9:12 AM, Stefan Eissing
>>> <st...@greenbytes.de> wrote:
>>>> With the timeout behaviour of SSL reads fixed, I am making another attempt at have http/2 connections behave properly in async MPMs, e.g. event. One thing for that necessary as change in server is the hook to send a last GOAWAY frame before the connection is closed. Not sending it seems to confuse browsers, leading to unwanted failure of the subsequent request.
>>>> 
>>>> You opinion therefore is required. On inspection of the connection shutdown handling, it seems that *all* MPM modules call, directly or indirectly, ap_start_lingering_close(). Keepalive connections are still healthy at this point and pool cleanup happens later, so this seems to be the place to install a new hook:
>>>> 
>>>> AP_DECLARE_HOOK(int,pre_close_connection,(conn_rec *c)) as RUN_ALL
>>>> 
>>>> Does this seem like the right place? Is the name any good?
>>>> 
>>>> (forget the question about naming, should not have disturbed the beast...)
>>> 
>>> Sounds reasonable, and c->pool cleanups you probably found are too
>>> late to write anything.
>> 
>> Indeed. All MPMs seem to cleanup pools way afterwards. Which is correct, I believe.
> 

Re: async keepalive in http/2

[Jim Jagielski <ji...@jaguNET.com>]

+1

> On Jan 22, 2016, at 9:17 AM, Stefan Eissing <st...@greenbytes.de> wrote:
> 
> 
>> Am 22.01.2016 um 15:16 schrieb Eric Covener <co...@gmail.com>:
>> 
>> On Fri, Jan 22, 2016 at 9:12 AM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>> With the timeout behaviour of SSL reads fixed, I am making another attempt at have http/2 connections behave properly in async MPMs, e.g. event. One thing for that necessary as change in server is the hook to send a last GOAWAY frame before the connection is closed. Not sending it seems to confuse browsers, leading to unwanted failure of the subsequent request.
>>> 
>>> You opinion therefore is required. On inspection of the connection shutdown handling, it seems that *all* MPM modules call, directly or indirectly, ap_start_lingering_close(). Keepalive connections are still healthy at this point and pool cleanup happens later, so this seems to be the place to install a new hook:
>>> 
>>> AP_DECLARE_HOOK(int,pre_close_connection,(conn_rec *c)) as RUN_ALL
>>> 
>>> Does this seem like the right place? Is the name any good?
>>> 
>>> (forget the question about naming, should not have disturbed the beast...)
>> 
>> Sounds reasonable, and c->pool cleanups you probably found are too
>> late to write anything.
> 
> Indeed. All MPMs seem to cleanup pools way afterwards. Which is correct, I believe.

Re: async keepalive in http/2

[Stefan Eissing <st...@greenbytes.de>]

> Am 22.01.2016 um 15:16 schrieb Eric Covener <co...@gmail.com>:
> 
> On Fri, Jan 22, 2016 at 9:12 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> With the timeout behaviour of SSL reads fixed, I am making another attempt at have http/2 connections behave properly in async MPMs, e.g. event. One thing for that necessary as change in server is the hook to send a last GOAWAY frame before the connection is closed. Not sending it seems to confuse browsers, leading to unwanted failure of the subsequent request.
>> 
>> You opinion therefore is required. On inspection of the connection shutdown handling, it seems that *all* MPM modules call, directly or indirectly, ap_start_lingering_close(). Keepalive connections are still healthy at this point and pool cleanup happens later, so this seems to be the place to install a new hook:
>> 
>> AP_DECLARE_HOOK(int,pre_close_connection,(conn_rec *c)) as RUN_ALL
>> 
>> Does this seem like the right place? Is the name any good?
>> 
>> (forget the question about naming, should not have disturbed the beast...)
> 
> Sounds reasonable, and c->pool cleanups you probably found are too
> late to write anything.

Indeed. All MPMs seem to cleanup pools way afterwards. Which is correct, I believe.

Re: async keepalive in http/2

[Eric Covener <co...@gmail.com>]

On Fri, Jan 22, 2016 at 9:12 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
> With the timeout behaviour of SSL reads fixed, I am making another attempt at have http/2 connections behave properly in async MPMs, e.g. event. One thing for that necessary as change in server is the hook to send a last GOAWAY frame before the connection is closed. Not sending it seems to confuse browsers, leading to unwanted failure of the subsequent request.
>
> You opinion therefore is required. On inspection of the connection shutdown handling, it seems that *all* MPM modules call, directly or indirectly, ap_start_lingering_close(). Keepalive connections are still healthy at this point and pool cleanup happens later, so this seems to be the place to install a new hook:
>
> AP_DECLARE_HOOK(int,pre_close_connection,(conn_rec *c)) as RUN_ALL
>
> Does this seem like the right place? Is the name any good?
>
> (forget the question about naming, should not have disturbed the beast...)

Sounds reasonable, and c->pool cleanups you probably found are too
late to write anything.