Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of trawick@gmail.com designates
 209.85.220.173 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <f6023cb29d354d6fa3aacfa34e759d4c@BY2PR03MB608.namprd03.prod.outlook.com>
References: 
 <31f247cbaff240b7a06a2c3713fb8a96@BY2PR03MB608.namprd03.prod.outlook.com>
	<f6023cb29d354d6fa3aacfa34e759d4c@BY2PR03MB608.namprd03.prod.outlook.com>
Date: Thu, 9 Aug 2012 15:52:44 -0400
Message-ID: 
 <CAKUrXK45qynd5bu=nShdWZSGQCzMP5A4HNKEtqDjgKKpG2wTPQ@mail.gmail.com>
Subject: Re: Fix for Windows bug#52476
From: Jeff Trawick <trawick@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

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 =91connect=92 or =91none=92, we read data from socket =
on worker
> thread. We use blocking recv and assign context->overlapped.Pointer to he=
ap
> allocated buffer. It is the same procedure as in case of =91AcceptFilter
> data=92, 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 =3D=3D NULL is not processe=
d
> correctly in windows version of httpd. It may be related to the fact that
> winnt_insert_network_bucket() rejects context records with
> overlapped.Pointer =3D=3D 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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- 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 =3D ap_get_module_config(c->conn_config,
                                                      &mpm_winnt_module);
-    if (context =3D=3D NULL || (e =3D context->overlapped.Pointer) =3D=3D =
NULL)
+    if (context =3D=3D NULL) {
         return AP_DECLINED;
+    }

-    /* seed the brigade with AcceptEx read heap bucket */
-    APR_BRIGADE_INSERT_HEAD(bb, e);
+    if ((e =3D context->overlapped.Pointer) !=3D NULL) {
+        /* seed the brigade with AcceptEx read heap bucket */
+        APR_BRIGADE_INSERT_HEAD(bb, e);
+    }
+
     /* also seed the brigade with the client socket. */
     e =3D 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
>
>


--=20
Born in Roswell... married an alien...
http://emptyhammock.com/