Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of trawick@gmail.com designates
 209.85.212.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAKUrXK6nMdqFQocR3dnN+8MV1f6Paprz0=mYRJW9iKzCza-cEw@mail.gmail.com>
References: 
 <31f247cbaff240b7a06a2c3713fb8a96@BY2PR03MB608.namprd03.prod.outlook.com>
	<f6023cb29d354d6fa3aacfa34e759d4c@BY2PR03MB608.namprd03.prod.outlook.com>
	<CAKUrXK45qynd5bu=nShdWZSGQCzMP5A4HNKEtqDjgKKpG2wTPQ@mail.gmail.com>
	<CAKUrXK7kg4m0x8UTNPgT=xzH9DSfKUA0X0v0fHx65FRePuKHgQ@mail.gmail.com>
	<CAKUrXK6nMdqFQocR3dnN+8MV1f6Paprz0=mYRJW9iKzCza-cEw@mail.gmail.com>
Date: Thu, 9 Aug 2012 18:35:21 -0400
Message-ID: 
 <CAKUrXK7-SDKWMB44NRtfwkurDc-xMbDYtXGAXY-5eX-gakadQA@mail.gmail.com>
Subject: Re: Fix for Windows bug#52476
From: Jeff Trawick <trawick@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

On Thu, Aug 9, 2012 at 5:50 PM, Jeff Trawick <trawick@gmail.com> wrote:
> On Thu, Aug 9, 2012 at 4:28 PM, Jeff Trawick <trawick@gmail.com> wrote:
>> On Thu, Aug 9, 2012 at 3:52 PM, Jeff Trawick <trawick@gmail.com> wrote:
>>> On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH)
>>> <claudioc@microsoft.com> wrote:
>>>> Better patch, fixed minor issue after another code review.
>>>
>>> I tested and seemed to get good results, but my testing isn't
>>> reproducible enough with/without various patches to be conclusive.
>>>
>>> A couple of comments on the patch itself:
>>>
>>> * Why is BytesRead initialized to 0 in the other function?  (I didn't
>>> include that part of your patch in my test.)
>>> * Pass apr_get_netos_error() to ap_log_error instead of rc after a
>>> Winsock function like recv() fails.  (apr_get_os_error() otherwise)
>>>
>>> More comments below:
>>>
>>>
>>>> Thanks
>>>>
>>>> Claudio
>>>
>>>> From: Claudio Caldato (MS OPEN TECH) [mailto:claudioc@microsoft.com]
>>>> Sent: Thursday, August 9, 2012 11:13 AM
>>>> To: dev@httpd.apache.org
>>>> Subject: Fix for Windows bug#52476
>>>>
>>>>
>>>>
>>>> Please code review the fix and let me know if you find any issue.
>>>>
>>>>
>>>>
>>>> Attached is the proposed patch for
>>>>
>>>> server\mpm\winnt\child.c
>>>>
>>>>
>>>>
>>>> Summary for code reviewers:
>>>>
>>>> If AcceptFilter is =91connect=92 or =91none=92, we read data from sock=
et on worker
>>>> thread. We use blocking recv and assign context->overlapped.Pointer to=
 heap
>>>> allocated buffer. It is the same procedure as in case of =91AcceptFilt=
er
>>>> data=92, but done in worker thread to keep listen thread unblocked.
>>>
>>> The big picture doesn't seem correct.  The main path of httpd (that
>>> stuff called via ap_run_process_connection()) needs to be able to read
>>> at will.  It shouldn't help to move a read() to before the call to
>>> ap_run_process_connection().
>>>
>>>
>>>> Note:
>>>>
>>>> It looks like context with overlapped.Pointer =3D=3D NULL is not proce=
ssed
>>>> correctly in windows version of httpd. It may be related to the fact t=
hat
>>>> winnt_insert_network_bucket() rejects context records with
>>>> overlapped.Pointer =3D=3D NULL
>>>
>>> Uhh, maybe I'm confused but that sounds like the issue!  (I'm not
>>> familiar with that hook.)
>>>
>>> What about this patch?
>>>
>>> Index: server/mpm/winnt/child.c
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- server/mpm/winnt/child.c    (revision 1371377)
>>> +++ server/mpm/winnt/child.c    (working copy)
>>> @@ -780,11 +780,15 @@
>>>      apr_bucket *e;
>>>      winnt_conn_ctx_t *context =3D ap_get_module_config(c->conn_config,
>>>                                                       &mpm_winnt_module=
);
>>> -    if (context =3D=3D NULL || (e =3D context->overlapped.Pointer) =3D=
=3D NULL)
>>> +    if (context =3D=3D NULL) {
>>>          return AP_DECLINED;
>>> +    }
>>>
>>> -    /* seed the brigade with AcceptEx read heap bucket */
>>> -    APR_BRIGADE_INSERT_HEAD(bb, e);
>>> +    if ((e =3D context->overlapped.Pointer) !=3D NULL) {
>>> +        /* seed the brigade with AcceptEx read heap bucket */
>>> +        APR_BRIGADE_INSERT_HEAD(bb, e);
>>> +    }
>>> +
>>>      /* also seed the brigade with the client socket. */
>>>      e =3D apr_bucket_socket_create(socket, c->bucket_alloc);
>>>      APR_BRIGADE_INSERT_TAIL(bb, e);
>>
>> no, that's wrong
>>
>> you only need to put the socket bucket there if you need to put data
>> in front of it; (when declining, core's insert_network_bucket hook
>> will do the right thing with the socket)
>>
>> AFAICT there's no functional issue with winnt_insert_network_bucket,
>> though it could defer to core to handle the socket bucket
>>
>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Please advise on what the next step(s) should be.
>>>>
>>>>
>>>>
>>>> Thanks
>>>>
>>>> Claudio
>
> Attached is an alternate version of your patch which uses a different
> control over how many bytes should be read.
>
> With TO_READ =3D 0 (disable your code), it falls over consistently.
> With TO_READ =3D 1 (read just 1 byte) it works reasonably consistently
> (1 handshake failure in 100s of attempted connections).
>
> Is there something about the state of the socket that gets reset once
> a vanilla recv() is performed?  (a little more later)
>
> wrowe had the suggestion recently that we weren't manufacturing the
> apr socket properly and the socket state was wrong.  I think that
> creation of the apr socket is as good as it can be, though in some
> experiments I did I was left nervous about this code on the listening
> socket:
>
>         /* The event needs to be removed from the accepted socket,
>          * if not removed from the listen socket prior to accept(),
>          */
>         rv =3D WSAEventSelect(nlsd, events[2], FD_ACCEPT);
>         if (rv) {
>             ap_log_error(APLOG_MARK, APLOG_ERR,
>                          apr_get_netos_error(), ap_server_conf, APLOGNO(0=
0333)
>                          "WSAEventSelect() failed.");
>             CloseHandle(events[2]);
>             return 1;
>         }
>
> This makes the listening socket non-blocking, and any attempt to set
> it blocking results in WSAEINVAL.  I dunno which of this leaks into
> the connected child, or if is related to the observation that the
> vanilla recv() somehow allows the apr-ized socket to work fine.
>
> We do run this code on the client socket for AcceptFilter None:
>
>             /* Per MSDN, cancel the inherited association of this socket
>              * to the WSAEventSelect API, and restore the state correspon=
ding
>              * to apr_os_sock_make's default assumptions (really, a flaw =
within
>              * os_sock_make and os_sock_put that it does not query).
>              */
>             WSAEventSelect(context->accept_socket, 0, 0);
>
> I guess this is all okay, but voodoo is all that comes to mind due to
> the first WSAEventSelect() magically making the socket non-blocking.

Woohoo!  I get consistent success with this bit of code in lieu of the
patch being discussed/tweaked in this thread.  I think this means that
simply calling recv() to change some aspect of the socket state is
what is needed, and the problem isn't related to the queuing of any
pre-read data.  (That's not to say that this is the desired way to
change that state, but it is better than nothing.)

#define TO_PEEK 1
        if (TO_PEEK > 0 &&
            (context->overlapped.Pointer =3D=3D NULL) &&
            (context->accept_socket !=3D INVALID_SOCKET)) {
            char buffer[TO_PEEK];

            rc =3D recv(context->accept_socket, buffer, TO_PEEK, MSG_PEEK);
            if (rc =3D=3D SOCKET_ERROR) {
                ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_netos_error(),
                             ap_server_conf, APLOGNO(00365)
                             "worker_main: recv(MSG_PEEK) error");
            }
        }

--=20
Born in Roswell... married an alien...
http://emptyhammock.com/