On Thu, 22 Aug 2024 at 08:42, Ruediger Pluem <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>> 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. > > 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. As for 1.7.x, I think that the recent fixes (r1920061, r1920070) should probably be enough for now. -- Ivan Zhakov
apr_socket_t behavior.pdf
Description: Adobe PDF document
