On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <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>> 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. Regards > > Rüdiger > > > > > I didn't dig into details, but non-blocking calls on Windows may > complete synchronously. > > > > > > Regards > > > > Rüdiger > > > > On 7/1/24 2:22 PM, Ruediger Pluem wrote: > > > Crossposting to dev@apr. Maybe some Windows folks want to > feedback. > > > > > > Regards > > > > > > Rüdiger > > > > > > On 6/20/24 12:12 PM, Ruediger Pluem wrote: > > >> > > >> > > >> On 6/19/24 2:52 PM, Joe Orton wrote: > > >>> On Wed, Jun 19, 2024 at 02:04:20PM +0200, Yann Ylavic wrote: > > >>>> On Wed, Jun 19, 2024 at 1:00 PM Ruediger Pluem < > rpl...@apache.org <mailto:rpl...@apache.org>> wrote: > > >>>>> As far as I read the code it does not. > > >>>>> > > >>>>> > https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/network_io/unix/sockets.c#L423-L433 > > >>>>> > > >>>>> We copy the data (sa, salen family and port) not a pointer. > > >>>> > > >>>> Ah yes, I was looking at win32 code, while Joe fixed it 13 > years ago > > >>>> for unix (r983618). > > >>>> So the pointer copy exists, but only for WIN32 and OS/2 AFAICT, > what a mess. > > >>> > > >>> Sorry! > > >>> > > >>>> Let me fix that then ;) > > >>> > > >>> There is a test in that commit, is it not catching the bug on > non-Unix? > > >> > > >> As far as I can tell the test is never hit on Windows as > apr_socket_connect(cd, sa) returns APR_SUCCESS > > >> despite being non blocking. > > >> > > >> > https://github.com/apache/apr/blob/b0a08c76ceacc2308d7cf1d5a7bb3c9b4665a432/test/testsock.c#L428-L435 > > >> > > >> .\testall.exe -v testsock > > >> testsock : |Line 433: Cannot test if connect completes > synchronously > > >> SUCCESS > > >> All tests passed. > > >> > > >> The following patch would fix this by causing the test to > executed and of course to fail: > > >> > > >> Index: network_io/win32/sockets.c > > >> > =================================================================== > > >> --- network_io/win32/sockets.c (revision 1917061) > > >> +++ network_io/win32/sockets.c (working copy) > > >> @@ -378,7 +378,7 @@ > > >> } > > >> > > >> if (rv == APR_FROM_OS_ERROR(WSAEWOULDBLOCK)) { > > >> - if (sock->timeout == 0) { > > >> + if (sock->timeout <= 0) { > > >> /* Tell the app that the connect is in progress... > > >> * Gotta play some games here. connect on Unix will > return > > >> * EINPROGRESS under the same circumstances that > Windows > > >> > > >> > > >> > > >> But as Windows is not my native environment I would appreciate > remote eyes if the fix is correct. > > >> > > >> > > > > > > > > > > > -- > > Ivan Zhakov > -- Ivan Zhakov
