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); > > > > > > Please advise on what the next step(s) should be. > > > > Thanks > > Claudio > > -- Born in Roswell... married an alien... http://emptyhammock.com/
