mpm_winnt lingering close

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

Steffen started testing and immediately found sth. Yeah! He sees many

[Mon Apr 03 14:20:28.390474 2017] [http2:warn] [pid 3180:tid 2880] [client 5.80.147.209:61342] AH10020: h2_session(106,DONE,0): session cleanup triggered by pool cleanup. this should have happened earlier already.

in his logs. The problem here is that mod_http2 would like to shutdown in ap_prep_lingering_close()
which is invoked by ap_lingering_close(). This is always done by mpm_event, worker and prefork AFAIK.

mpm_winnt has an optimization in line 814:
        ap_process_connection(c, context->sock);

        apr_socket_opt_get(context->sock, APR_SO_DISCONNECTED, &disconnected);

        if (!disconnected) {
            context->accept_socket = INVALID_SOCKET;
            if (!c->aborted) { 
                ap_lingering_close(c);
            }
        }

So, if the connection is already gone or was aborted, it will not call
this and the "safe" shutdown is not triggered. "safe" insofar as
mod_http2 needs to shutdown the connection *before* mod_ssl does.

Question is: do we "fix" mpm_winnt or is there a better way for mod_http2 to shutdown the connection before mod_ssl does. This would need to work in async mpms for any connection state.

-Stefan

Re: mpm_winnt lingering close

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

> Am 03.04.2017 um 21:17 schrieb William A Rowe Jr <wr...@rowe-clan.net>:
> 
> On Mon, Apr 3, 2017 at 8:21 AM, Eric Covener <co...@gmail.com> wrote:
>> On Mon, Apr 3, 2017 at 9:07 AM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>> Question is: do we "fix" mpm_winnt or is there a better way for mod_http2 to shutdown the connection before mod_ssl does. This would need to work in async mpms for any connection state.
>> 
>> I think it's okay to add the prep cal when short-circuiting lingering
>> close, but it seems like very little extra will be running in the full
>> call either.
> 
> Jeff Trawick knew this bit of logic better than most any of us, I'd love
> to hear his thoughts on the cleanest solution. But fixing mpm_winnt
> to behave as the other MPM's would be worthwhile. It's also worth
> looking at third party MPM's such as mpm_itk to see if we have an
> underlying bug that must be fixed.
> 
> I presume we still allow the disconnected socket to be recycled, which
> was the underlying idea behind the shortcut/optimization. It seems that
> the shortcut isn't (valid).

All other of our mpms in trunk call ap_lingering_close() unconditionally
after ap_process_connection(). So, mod_http2 should be fine there.

mpm_itk, in the latest release at least, also always invokes 
ap_lingering_close() before terminating a connection.


I propose the following patch for trunk and, if it does indeed work 
as expected will propose backport to 2.4.x. Since I do not run a
Windows build, I'd like to hear peoples opinion and need a tester. 

Index: server/mpm/winnt/child.c
===================================================================
--- server/mpm/winnt/child.c	(revision 1790842)
+++ server/mpm/winnt/child.c	(working copy)
@@ -817,10 +817,8 @@
 
         if (!disconnected) {
             context->accept_socket = INVALID_SOCKET;
-            if (!c->aborted) { 
-                ap_lingering_close(c);
-            }
         }
+        ap_lingering_close(c);
     }
 
     ap_update_child_status_from_indexes(0, thread_num, SERVER_DEAD, NULL);

Steffen, if you want to try this in your Windows build, I attached the 
file from 2.4.x with the patch applied.

Re: mpm_winnt lingering close

[William A Rowe Jr <wr...@rowe-clan.net>]

On Mon, Apr 3, 2017 at 8:21 AM, Eric Covener <co...@gmail.com> wrote:
> On Mon, Apr 3, 2017 at 9:07 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>> Question is: do we "fix" mpm_winnt or is there a better way for mod_http2 to shutdown the connection before mod_ssl does. This would need to work in async mpms for any connection state.
>
> I think it's okay to add the prep cal when short-circuiting lingering
> close, but it seems like very little extra will be running in the full
> call either.

Jeff Trawick knew this bit of logic better than most any of us, I'd love
to hear his thoughts on the cleanest solution. But fixing mpm_winnt
to behave as the other MPM's would be worthwhile. It's also worth
looking at third party MPM's such as mpm_itk to see if we have an
underlying bug that must be fixed.

I presume we still allow the disconnected socket to be recycled, which
was the underlying idea behind the shortcut/optimization. It seems that
the shortcut isn't (valid).

Re: mpm_winnt lingering close

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

On Mon, Apr 3, 2017 at 9:07 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
> Question is: do we "fix" mpm_winnt or is there a better way for mod_http2 to shutdown the connection before mod_ssl does. This would need to work in async mpms for any connection state.

I think it's okay to add the prep cal when short-circuiting lingering
close, but it seems like very little extra will be running in the full
call either.

-- 
Eric Covener
covener@gmail.com