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 <rpl...@apache.org
>> <mailto:rpl...@apache.org>> 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 <rpl...@apache.org
>> <mailto:rpl...@apache.org> <mailto:rpl...@apache.org
>> <mailto:rpl...@apache.org>>> wrote:
>> >>
>> >>
>> >>
>> >> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
>> >> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <i...@apache.org
>> <mailto:i...@apache.org> <mailto:i...@apache.org
>> <mailto:i...@apache.org>> <mailto:i...@apache.org
>> <mailto:i...@apache.org>
>> >> <mailto:i...@apache.org <mailto:i...@apache.org>>>> wrote:
>> >> >
>> >> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem
>> <rpl...@apache.org <mailto:rpl...@apache.org>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>
>> >> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>>> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
>> >> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem
>> <rpl...@apache.org <mailto:rpl...@apache.org>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>
>> >> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>
>> >> > <mailto:rpl...@apache.org <mailto:rpl...@apache.org>
>> <mailto:rpl...@apache.org <mailto:rpl...@apache.org>>>>>
>> 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
