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>> 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>>> 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>>> 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>>>> 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.
Regards
Rüdiger
