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 >> >> > > > > -- > Born in Roswell... married an alien... > http://emptyhammock.com/ -- Born in Roswell... married an alien... http://emptyhammock.com/
