You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2024/07/01 12:22:17 UTC
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
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 <rp...@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.
>
>
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Aug 21, 2024 at 04:19:30PM +0200, Ivan Zhakov wrote:
> Do you plan to backport r1920070 to the APR 1.7.x branch?
It looks good in CI, so yes I'll backup r1920061, r1920070 for 1.7.x
when the current release cycle is done.
Regards, Joe
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov via dev <de...@apr.apache.org>.
On Tue, 20 Aug 2024 at 18:11, Joe Orton <jo...@redhat.com> wrote:
> On Tue, Aug 20, 2024 at 03:45:05PM +0200, Ivan Zhakov wrote:
> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <iv...@apache.org> wrote:
> > > 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),
>
> Following up here too for completeness - I agree this is right, I
> committed that in r1920070 with some wording changes too.
>
> Thanks Ivan & thanks Ruediger for following up on this issue.
>
> Regards, Joe
>
> Hi Joe,
Do you plan to backport r1920070 to the APR 1.7.x branch?
--
Ivan Zhakov
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Aug 20, 2024 at 03:45:05PM +0200, Ivan Zhakov wrote:
> On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <iv...@apache.org> wrote:
> > 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),
Following up here too for completeness - I agree this is right, I
committed that in r1920070 with some wording changes too.
Thanks Ivan & thanks Ruediger for following up on this issue.
Regards, Joe
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 8/20/24 2:18 PM, Ivan Zhakov wrote:
> On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>
>
>
> On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
> <ma...@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?
Yes :-). This is how I discovered the bug, because we thought that with the current implementation
of apr_socket_create on Windows this test should fail on Windows as above, but it was never executed,
because of that issue that I try to fix with the test above. Whole story is here:
https://lists.apache.org/thread/hfb49vlqh4c3gxmzryls1qdpghwcycsj
In order to fix this test failure after applying the patch we need to do a similar thing for Windows
that was doen for Unix in https://svn.apache.org/viewvc?view=revision&revision=983618
>
> I hope this helps.
Yes.
Regards
Rüdiger
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
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 <rpluem@apache.org <ma...@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 <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
>> <ma...@apache.org>>> wrote:
>> >>
>> >>
>> >>
>> >> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
>> >> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <ma...@apache.org> <mailto:ivan@apache.org
>> <ma...@apache.org>> <mailto:ivan@apache.org <ma...@apache.org>
>> >> <mailto:ivan@apache.org <ma...@apache.org>>>> wrote:
>> >> >
>> >> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
>> <mailto:rpluem@apache.org <ma...@apache.org>> <mailto:rpluem@apache.org <ma...@apache.org>
>> >> <mailto:rpluem@apache.org <ma...@apache.org>>>> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
>> >> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
>> <mailto:rpluem@apache.org <ma...@apache.org>>
>> >> <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@apache.org>>>
>> <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@apache.org>>
>> >> > <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 8/22/24 1:17 PM, Ivan Zhakov wrote:
> On Thu, 22 Aug 2024 at 08:42, Ruediger Pluem <rpluem@apache.org <ma...@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 <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
> <ma...@apache.org>>> wrote:
> >>
> >>
> >>
> >> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
> >> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <ma...@apache.org> <mailto:ivan@apache.org
> <ma...@apache.org>> <mailto:ivan@apache.org <ma...@apache.org>
> >> <mailto:ivan@apache.org <ma...@apache.org>>>> wrote:
> >> >
> >> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
> <mailto:rpluem@apache.org <ma...@apache.org>> <mailto:rpluem@apache.org <ma...@apache.org>
> >> <mailto:rpluem@apache.org <ma...@apache.org>>>> wrote:
> >> >
> >> >
> >> >
> >> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> >> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
> <mailto:rpluem@apache.org <ma...@apache.org>>
> >> <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@apache.org>>>
> <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@apache.org>>
> >> > <mailto:rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org <ma...@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.
>
> As for 1.7.x, I think that the recent fixes (r1920061, r1920070) should probably be enough for now.
Fair enough :-)
Regards
Rüdiger
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov <iv...@apache.org>.
On Thu, 22 Aug 2024 at 08:42, Ruediger Pluem <rp...@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 <rpluem@apache.org
> <ma...@apache.org>> wrote:
> >>
> >>
> >>
> >> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
> >> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org
> <ma...@apache.org> <mailto:ivan@apache.org
> >> <ma...@apache.org>>> wrote:
> >> >
> >> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <
> rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
> >> <ma...@apache.org>>> wrote:
> >> >
> >> >
> >> >
> >> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> >> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <
> rpluem@apache.org <ma...@apache.org>
> >> <mailto:rpluem@apache.org <ma...@apache.org>> <mailto:
> rpluem@apache.org <ma...@apache.org>
> >> > <mailto:rpluem@apache.org <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
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 <rpluem@apache.org <ma...@apache.org>> wrote:
>>
>>
>>
>> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
>> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <ma...@apache.org> <mailto:ivan@apache.org
>> <ma...@apache.org>>> wrote:
>> >
>> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
>> <ma...@apache.org>>> wrote:
>> >
>> >
>> >
>> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
>> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
>> <mailto:rpluem@apache.org <ma...@apache.org>> <mailto:rpluem@apache.org <ma...@apache.org>
>> > <mailto:rpluem@apache.org <ma...@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.
Regards
Rüdiger
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 8/20/24 6:15 PM, Ivan Zhakov via dev wrote:
> On Tue, 20 Aug 2024 at 17:40, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>
>
>
> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <ma...@apache.org> <mailto:ivan@apache.org
> <ma...@apache.org>>> wrote:
> >
> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
> <ma...@apache.org>>> wrote:
> >
> >
> >
> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>
> <mailto:rpluem@apache.org <ma...@apache.org>> <mailto:rpluem@apache.org <ma...@apache.org>
> > <mailto:rpluem@apache.org <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov via dev <de...@apr.apache.org>.
On Tue, 20 Aug 2024 at 17:40, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 8/20/24 3:45 PM, Ivan Zhakov wrote:
> > On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <mailto:
> ivan@apache.org>> wrote:
> >
> > On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org
> <ma...@apache.org>> wrote:
> >
> >
> >
> > On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> > > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org
> <ma...@apache.org> <mailto:rpluem@apache.org
> > <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 8/20/24 3:45 PM, Ivan Zhakov wrote:
> On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <ivan@apache.org <ma...@apache.org>> wrote:
>
> On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>
>
>
> On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@apache.org> <mailto:rpluem@apache.org
> <ma...@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?
Regards
Rüdiger
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov <iv...@apache.org>.
On Tue, 20 Aug 2024 at 14:18, Ivan Zhakov <iv...@apache.org> wrote:
> On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rp...@apache.org> wrote:
>
>>
>>
>> On 8/20/24 1:32 PM, Ivan Zhakov wrote:
>> > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <mailto:
>> rpluem@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 r
1920061 <https://svn.apache.org/r1920061>.
--
Ivan Zhakov
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov <iv...@apache.org>.
On Tue, 20 Aug 2024 at 13:47, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> > On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <mailto:
> rpluem@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 <
> rpluem@apache.org <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 8/20/24 1:32 PM, Ivan Zhakov wrote:
> On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rpluem@apache.org <ma...@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.
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 <rpluem@apache.org <ma...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ivan Zhakov <iv...@apache.org>.
On Fri, 9 Aug 2024 at 08:29, Ruediger Pluem <rp...@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?
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 <rp...@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
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
Anyone?
Regards
Rüdiger
On 8/9/24 8:28 AM, Ruediger Pluem 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?
>
> 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 <rp...@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.
>>>
>>>
>>
>
Re: svn commit: r1918412 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
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?
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 <rp...@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.
>>
>>
>