Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c

On 8/20/24 2:18 PM, Ivan Zhakov wrote:
> On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <[email protected] <mailto:[email protected]>> wrote:
> 
> 
> 
>     On 8/20/24 1:32 PM, Ivan Zhakov wrote:
>     > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <[email protected] <mailto:[email protected]> <mailto:[email protected]
>     <mailto:[email protected]>>> wrote:
>     >
>     >     Any APR windows guy on the below?
>     >
>     >     On Windows apr_socket_connect(cd, sa) returns APR_SUCCESS despite being non blocking.
>     >     This doesn't sound correct. Can someone have a look on the patch?
>     >
>     > Which patch do you mean r1918412 or something else?
> 
>     The patch below in this mail.
> 
> Ok, thanks!
> 
> So what is happening in my environment in testsock:test_get_addr() on Windows:
> 1. Call to apr_socket_create() sets timeout to -1. This means "block indefinitely" as far as I understand. See apr_socket_wait()
> implementation as an example.
> 2. Call to apr_socket_opt_set(APR_SO_NONBLOCK, 1) calls ioctlsocket(FIONBIO, 1) and DOES NOT update sock->timeout
> 3. connect() returns WSAEWOULDBLOCK
> 4. At this time sock->timeout == -1
> 
> I am not an expert in apr_socket_t implementation. But I see the following:
> 1. apr_socket_t has separate timeout and non-blocking flags.
> 2. apr_socket_opt_set() doesn't change sock->timeout on Unix
> <https://github.com/apache/apr/blob/cd3698c985708920d9369eb5db98070c0d78e2aa/network_io/unix/sockopt.c#L182> and Windows
> <https://github.com/apache/apr/blob/cd3698c985708920d9369eb5db98070c0d78e2aa/network_io/win32/sockopt.c#L156>.
> 3. apr_socket_timeout() updates timeout AND non-blocking on Unix
> <https://github.com/apache/apr/blob/cd3698c985708920d9369eb5db98070c0d78e2aa/network_io/unix/sockopt.c#L75> and Windows
> <https://github.com/apache/apr/blob/cd3698c985708920d9369eb5db98070c0d78e2aa/network_io/win32/sockopt.c#L53>.
> 
> I don't know what was the idea of having separate timeout value and non-blocking flag, but the proposed patch doesn't seem correct.
> 
> Easy solution is to use apr_socket_timeout() in the test:
> [[[
> Index: test/testsock.c
> ===================================================================
> --- test/testsock.c (revision 1920036)
> +++ test/testsock.c (working copy)
> @@ -420,7 +420,7 @@
>      APR_ASSERT_SUCCESS(tc, "create client socket", rv);
>  
>      APR_ASSERT_SUCCESS(tc, "enable non-block mode",
> -                       apr_socket_opt_set(cd, APR_SO_NONBLOCK, 1));
> +                       apr_socket_timeout_set(cd, 0));
>  
>      /* It is valid for a connect() on a socket with NONBLOCK set to
>       * succeed (if the connection can be established synchronously),
> 
>  ]]]
> 
> With this patch test starts failing with the following error:
> [[[
>   Message: 
> Line 471: expected <000001BEF3EBD028>, but saw <000001BEF3EA13C8>
> 
>   Stack Trace: 
> testsock line 675
> ]]
> 
> Is it expected?

Yes :-). This is how I discovered the bug, because we thought that with the current implementation
of apr_socket_create on Windows this test should fail on Windows as above, but it was never executed,
because of that issue that I try to fix with the test above. Whole story is here:
https://lists.apache.org/thread/hfb49vlqh4c3gxmzryls1qdpghwcycsj

In order to fix this test failure after applying the patch we need to do a similar thing for Windows
that was doen for Unix in https://svn.apache.org/viewvc?view=revision&revision=983618

> 
> I hope this helps.

Yes.

Regards

Rüdiger