On Thu, Aug 9, 2012 at 5:50 PM, Jeff Trawick <[email protected]> wrote:
> On Thu, Aug 9, 2012 at 4:28 PM, Jeff Trawick <[email protected]> wrote:
>> On Thu, Aug 9, 2012 at 3:52 PM, Jeff Trawick <[email protected]> wrote:
>>> On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH)
>>> <[email protected]> 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:[email protected]]
>>>> Sent: Thursday, August 9, 2012 11:13 AM
>>>> To: [email protected]
>>>> 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 ‘connect’ or ‘none’, we read data from socket 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 ‘AcceptFilter
>>>> data’, 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 == NULL is not processed
>>>> correctly in windows version of httpd. It may be related to the fact that
>>>> winnt_insert_network_bucket() rejects context records with
>>>> overlapped.Pointer == 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
>>> ===================================================================
>>> --- 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 = ap_get_module_config(c->conn_config,
>>> &mpm_winnt_module);
>>> - if (context == NULL || (e = context->overlapped.Pointer) == NULL)
>>> + if (context == NULL) {
>>> return AP_DECLINED;
>>> + }
>>>
>>> - /* seed the brigade with AcceptEx read heap bucket */
>>> - APR_BRIGADE_INSERT_HEAD(bb, e);
>>> + if ((e = context->overlapped.Pointer) != NULL) {
>>> + /* seed the brigade with AcceptEx read heap bucket */
>>> + APR_BRIGADE_INSERT_HEAD(bb, e);
>>> + }
>>> +
>>> /* also seed the brigade with the client socket. */
>>> e = 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 = 0 (disable your code), it falls over consistently.
> With TO_READ = 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 = WSAEventSelect(nlsd, events[2], FD_ACCEPT);
> if (rv) {
> ap_log_error(APLOG_MARK, APLOG_ERR,
> apr_get_netos_error(), ap_server_conf, APLOGNO(00333)
> "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 corresponding
> * 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 == NULL) &&
(context->accept_socket != INVALID_SOCKET)) {
char buffer[TO_PEEK];
rc = recv(context->accept_socket, buffer, TO_PEEK, MSG_PEEK);
if (rc == SOCKET_ERROR) {
ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_netos_error(),
ap_server_conf, APLOGNO(00365)
"worker_main: recv(MSG_PEEK) error");
}
}
--
Born in Roswell... married an alien...
http://emptyhammock.com/
