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

On 8/22/24 4:47 PM, Ruediger Pluem wrote:
> 
> 
> On 8/22/24 1:17 PM, Ivan Zhakov wrote:
>> On Thu, 22 Aug 2024 at 08:42, Ruediger Pluem <[email protected] <mailto:[email protected]>> wrote:
>>
>>
>>
>>     On 8/21/24 9:30 AM, Ruediger Pluem wrote:
>>     >
>>     >
>>     > On 8/20/24 6:15 PM, Ivan Zhakov via dev wrote:
>>     >> On Tue, 20 Aug 2024 at 17:40, Ruediger Pluem <[email protected] <mailto:[email protected]> <mailto:[email protected]
>>     <mailto:[email protected]>>> wrote:
>>     >>
>>     >>
>>     >>
>>     >>     On 8/20/24 3:45 PM, Ivan Zhakov wrote:
>>     >>     > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <[email protected] <mailto:[email protected]> <mailto:[email protected]
>>     <mailto:[email protected]>> <mailto:[email protected] <mailto:[email protected]>
>>     >>     <mailto:[email protected] <mailto:[email protected]>>>> wrote:
>>     >>     >
>>     >>     >     On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <[email protected] <mailto:[email protected]>
>>     <mailto:[email protected] <mailto:[email protected]>> <mailto:[email protected] <mailto:[email protected]>
>>     >>     <mailto:[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]>>
>>     >>     <mailto:[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>>
>>     <mailto:[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>
>>     >>     >         <mailto:[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?
>>     >>     >
>>     >>     >     I hope this helps.
>>     >>     >
>>     >>     > I fixed the issue with the result lifetime of apr_socket_addr_get() in r1920061 <https://svn.apache.org/r1920061>.
>>     >>
>>     >>     Thanks. Hence my patch is fine from your point of view?
>>     >>
>>     >> As far as I understand the idea about apr_socket_t timeout and non-blocking flag the proposed patch with change condition to
>>     >> `sock->timeout <= 0` is not correct: negative timeout means infinite timeout. So blocking apr_socket_t should wait
>>     indefinitely. A
>>     >> potential solution would be to check for `apr_is_option_set(sock, APR_SO_NONBLOCK)` but I am not sure about this.
>>     >
>>     > Call me stubborn, but with this approach we have a different behavior of apr_socket_connect between Unix and Windows.
>>     > If the socket is set to non blocking via apr_is_option_set(sock, APR_SO_NONBLOCK) but the timeout is still -1 we have the
>>     > following results:
>>     >
>>     > On Unix: Return APR_EINPROGRESS
>>     > On Windows: Return APR_SUCCESS
>>     >
>>     > If you think that the behavior on Windows is correct, then we should change it on Unix to match the one on Windows.
>>     > I have a hard time finding an argument why Unix and Windows should behave differently in the same situation.
>>
>>     I would understand if this different behavior should be kept for 1.7 or even 1.8. But I think in trunk they should behave the the
>>     same.
>>
>> I agree that Unix and Windows behavior should align, but the situation is complicated by the differences in how
>> apr_socket_timeout_set() and apr_socket_opt_set(sock, APR_SO_NONBLOCK) work on these platforms. So if we're to change that, I
>> think that both apr_socket_connect() and apr_socket_timeout_set() would need to be addressed together.
>>
>> I've attached a preliminary analysis of the current behavior. I may be able to take a further look at this, although I can't
>> promise it.
> 
> Thanks for generating this overview. This looks kind of nasty and messy. This should be cleaned up such that we have the same
> behavior.

But I guess we cannot do that in 1.7.x, probably not even in 1.8.x.

Regards

Rüdiger