On Tue, 20 Aug 2024 at 17:40, Ruediger Pluem <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>> wrote: > > > > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <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>>> 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. Given the fact that we are going to backport these changes to the stable branch (1.7.x.) I propose the following: 1. Backport r1920061: I think it's safe because we already have the same code on Unix. 2. Commit and backport my patch that changes apr_socket_opt_set() to apr_socket_timeout_set() in test/testsock.c. -- Ivan Zhakov
