On Thu, Aug 9, 2012 at 5:50 PM, Jeff Trawick <traw...@gmail.com> wrote: > 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.
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/