httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: Fix for Windows bug#52476
Date Thu, 09 Aug 2012 19:52:44 GMT
On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH)
<claudioc@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:claudioc@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/

Mime
View raw message