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.
Index: server/mpm/winnt/child.c
===================================================================
--- server/mpm/winnt/child.c (revision 1371377)
+++ server/mpm/winnt/child.c (working copy)
@@ -832,6 +832,31 @@
}
}
+#define TO_READ 1
+ /* AcceptFilter connect|none do not read data. Read it here. */
+ if (TO_READ > 0 &&
+ (context->overlapped.Pointer == NULL) &&
+ (context->accept_socket != INVALID_SOCKET)) {
+ apr_bucket *b;
+ char *buffer;
+
+ buffer = apr_bucket_alloc(TO_READ, context->ba);
+ rc = recv(context->accept_socket, buffer, TO_READ, 0);
+ if (rc == SOCKET_ERROR)
+ {
+ ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_netos_error(),
+ ap_server_conf, APLOGNO(00365)
+ "worker_main: recv error");
+ apr_bucket_free(buffer);
+ continue;
+ }
+
+ b = apr_bucket_heap_create(buffer, rc,
+ apr_bucket_free, context->ba);
+ b->length = rc;
+ context->overlapped.Pointer = b;
+ }
+
e = context->overlapped.Pointer;
ap_create_sb_handle(&sbh, context->ptrans, 0, thread_num);
