On Thu, Aug 9, 2012 at 4:28 PM, Jeff Trawick <traw...@gmail.com> wrote: > On Thu, Aug 9, 2012 at 3:52 PM, Jeff Trawick <traw...@gmail.com> wrote: >> On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH) >> <claud...@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:claud...@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 ‘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);