On Fri, Aug 10, 2012 at 1:31 PM, Jeff Trawick <traw...@gmail.com> wrote:
> On Fri, Aug 10, 2012 at 12:41 PM, Jeff Trawick <traw...@gmail.com> wrote:
>> On Thu, Aug 9, 2012 at 7:35 PM, William A. Rowe Jr. <wr...@rowe-clan.net>
>> wrote:
>>> On 8/9/2012 11:26 AM, Claudio Caldato (MS OPEN TECH) wrote:
>>>> Better patch, fixed minor issue after another code review.
>>>
>>> Sadly, it won't fix the defect.
>>>
>>> Yes, you are successfullly performing a blocking initial read.
>>>
>>> And the pipe remains unblocked for the rest of the connection, so any
>>> further blocking reads will be ignored (just like the initial read by
>>> mod_ssl failed to block). Any module expecting blocking behavior on
>>> subsequent reads will be disappointed.
>>
>> The SSL case with the chitchat to set up the connection is a bit
>> different than the simple case where as soon as you read you have
>> everything needed, so it does seem moderately interesting that the
>> read|peek makes the simple SSL testcase work.
>>
>> Also in the FWLIW department:
>>
>> a. once we get to the process_connection hook, APR's limited knowledge
>> of the socket state (timeout, non-blocking) is the same on Linux and
>> Windows
>>
>> (That was expected all along.)
>>
>> I had hoped to query state directly from the OS socket too, but
>> apparently the Microsoft folks don't think you need to be able to do
>> that, and I don't see any external sysinternals|other tools to tell
>> you anything interesting about the socket other than the normal
>> netstat stuff.
>>
>> b. Using WSAEnumNetworkEvents to clear any existing events didn't seem to
>> help
>>
>> /* start new */
>> rv = WSAEnumNetworkEvents(context->accept_socket, 0,
>> &lpNetworkEvents);
>> if (rv == SOCKET_ERROR) {
>> ap_log_error(APLOG_MARK, APLOG_WARNING, 0, ap_server_conf,
>> "failed to clear network events on connected
>> socket");
>> }
>> /* end new */
>> apr_os_sock_make(&context->sock, &sockinfo, context->ptrans);
>>
>> c. switching out the WSAEvent stuff for a straight
>> select(is-listener-readable) doesn't work around the problem either
>
> The APR socket gets set to non-blocking prior to the first call to
> apr_socket_recv():
>
> #0 apr_socket_opt_set (sock=0x2933890, opt=8, on=1)
> at network_io/win32/sockopt.c:104
> #1 0x00423daa in ap_core_output_filter (f=0xc2bb310, new_bb=0x2933d20)
> at core_filters.c:394
> #2 0x0042197c in ap_pass_brigade (next=0xc2bb310, bb=0x2933d20)
> at util_filter.c:533
> #3 0x0045be1e in bio_filter_out_pass (bio=<value optimized out>)
> at ssl_engine_io.c:134
> #4 bio_filter_out_flush (bio=<value optimized out>) at ssl_engine_io.c:155
> #5 0x0045bff9 in bio_filter_in_read (bio=0x28aa9a8,
> in=0x2899348 "0╪ \f\003\001", inlen=11) at ssl_engine_io.c:458
> #6 0x004b30f7 in BIO_read ()
> #7 0x00c1b798 in ?? ()
>
> We picked up that apr_socket_opt_set() from the async-dev branch with
> r327872, though the timeout calls in there were changed subsequently.
> I wonder if that call is stray and it doesn't get along with the
> timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
> use on Windows, in lieu of non-blocking socket + poll like on Unix.
>
> At the time it was added, the new code was
>
> apr_socket_timeout_set(client_socket, 0)
> apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)
>
> (redundant unless there was some APR glitch at the time)
>
> The apr_socket_timeout_set() was subsequently removed.
>
> I'll proceed with the obvious test. (I'm pretending that someone is
> out there reading this and will chime in with any hints or other
> comments based on my stream of observations :) )
This patch is testing great so far with the AcceptFilter-none+SSL
scenario on Windows.
Index: server/core_filters.c
===================================================================
--- server/core_filters.c (revision 1371377)
+++ server/core_filters.c (working copy)
@@ -391,10 +391,6 @@
if (ctx == NULL) {
ctx = apr_pcalloc(c->pool, sizeof(*ctx));
net->out_ctx = (core_output_filter_ctx_t *)ctx;
- rv = apr_socket_opt_set(net->client_socket, APR_SO_NONBLOCK, 1);
- if (rv != APR_SUCCESS) {
- return rv;
- }
/*
* Need to create tmp brigade with correct lifetime. Passing
* NULL to apr_brigade_split_ex would result in a brigade
I'll run it through the test framework on Linux before committing.
--
Born in Roswell... married an alien...
http://emptyhammock.com/
