Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Wed, 29 Dec 2021 at 19:53, <yl...@apache.org> wrote:
>
> Author: ylavic
> Date: Wed Dec 29 16:53:51 2021
> New Revision: 1896510
>
> URL: http://svn.apache.org/viewvc?rev=1896510&view=rev
> Log:
> Merge r1895106, r1895111, r1895175, r1895181, r1895465 from trunk:
>
>
> Fix drain wakeup pipe issue when multiple threads call apr_pollset_wakeup/apr_pollcb_wakeup for the same pollset filling up drain pipe. Use atomics so that wakeup call is noop if some other thread allready done this
>
>
> Follow up to r1895106: now we want blocking reads on unix too so revert r1894914.
>
>
> Follow up to r1895106: Use less expensive atomics for wakeup.
>
> If pipe writers (wakeup) put a single byte until it's consumed by the
> reader (drain), it's enough to use an atomic cas for the writers (still) and
> an atomic (re)set for the reader (no more cas here).
>
> This requires that the reader never blocks on read though (e.g. spurious return
> from poll), so make the read side on the pipe non-blocking again/finally.
>
> Since synchronous non-blocking read is not a thing for Windows' Readfile(), add
> a ->socket flag to this arch's apr_file_t (like the existing ->pipe one) which
> file_socket_pipe_create() will set to make apr_file_read/write() handle
> non-blocking (nor overlapped) socket pipes with WSARecv/Send().
>
>
> Use enum instead multiple booleans
>
>
> Fix remaining change when apr_filetype_e was added
>
>
Hi Yann,

This change does not compile on Windows in APR 1.7.x:
[[[
file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
point to struct/union
file_io\win32\readwrite.c(325): warning C4047: 'function': 'SOCKET'
differs in levels of indirection from 'WSABUF *'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 1
file_io\win32\readwrite.c(325): warning C4047: 'function': 'LPWSABUF'
differs in levels of indirection from 'int'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 2
file_io\win32\readwrite.c(325): warning C4047: 'function': 'DWORD'
differs in levels of indirection from 'DWORD *'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 3
file_io\win32\readwrite.c(326): warning C4047: 'function': 'LPDWORD'
differs in levels of indirection from 'DWORD'
file_io\win32\readwrite.c(326): warning C4024: 'WSASend': different
types for formal and actual parameter 4
file_io\win32\readwrite.c(326): warning C4047: 'function': 'DWORD'
differs in levels of indirection from 'void *'
file_io\win32\readwrite.c(326): warning C4024: 'WSASend': different
types for formal and actual parameter 5
file_io\win32\readwrite.c(325): error C2198: 'WSASend': too few
arguments for call
]]]

I also have a high-level objection against backporting this change to
APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
regression fixes should be backported to the stable branch. r1896510
is a significant change and as far as I understand it's not a
regression fix. So I think it would be better to revert r1896510 and
release it as part of APR 2.0 (or 1.8.x).


--
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Eric Covener <co...@gmail.com>]

> I also have a high-level objection against backporting this change to
> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> regression fixes should be backported to the stable branch. r1896510
> is a significant change and as far as I understand it's not a
> regression fix. So I think it would be better to revert r1896510 and
> release it as part of APR 2.0 (or 1.8.x).

No comment on the scale/significance here, but IMO fixes to
non-regression defects are suitable for a patch/micro releases of APR.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[William A Rowe Jr <wr...@rowe-clan.net>]

The answer to your question is whether a consumer who built against
apr<1.7.0 is going to blow up, whether they borrowed "private" API's
or not. If they were exported, it's effectively public.

Or, whether a consumer built against 1.7.1 would blow up against 1.7.0
- if that's true, we need to revert.

Those are the last of my obvious observations, I'll need to diff the
resulting include/... tree between 1.7.0 and 1.7.x branch to know for
sure, but thanks everyone for reviewing these questions.

Cheers,

Bill

On Tue, Feb 15, 2022 at 7:24 AM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Wed, 9 Feb 2022 at 13:47, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com> wrote:
>>>
>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>
>>> > This part is now in the following branch:
>>> > https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>>> >
>>> > What do you think?
>>> >
>>> > It would be great if someone could take a look on the implementation from
>>> > the *nix perspective.
>>> > After that, I propose to merge the branch into trunk.
>>>
>>> In case this helps, I have tested the branch on Ubuntu x64 and it seems
>>> to compile and pass the tests.
>>>
>>>
>>
>> Thanks! Merged branch to trunk on r1897895.
>>
>> Backporting changes to 1.8.x in my TODO list.
>>
> Nominated this change to 1.8.x branch. Please review.
>
> --
> Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Wed, 9 Feb 2022 at 13:47, Ivan Zhakov <iv...@visualsvn.com> wrote:

> On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com>
> wrote:
>
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>> > This part is now in the following branch:
>> >
>> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>> >
>> > What do you think?
>> >
>> > It would be great if someone could take a look on the implementation
>> from
>> > the *nix perspective.
>> > After that, I propose to merge the branch into trunk.
>>
>> In case this helps, I have tested the branch on Ubuntu x64 and it seems
>> to compile and pass the tests.
>>
>>
>>
> Thanks! Merged branch to trunk on r1897895.
>
> Backporting changes to 1.8.x in my TODO list.
>
> Nominated this change to 1.8.x branch. Please review.

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com>
wrote:

> Ivan Zhakov <iv...@visualsvn.com> writes:
>
> > This part is now in the following branch:
> >
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
> >
> > What do you think?
> >
> > It would be great if someone could take a look on the implementation from
> > the *nix perspective.
> > After that, I propose to merge the branch into trunk.
>
> In case this helps, I have tested the branch on Ubuntu x64 and it seems
> to compile and pass the tests.
>
>
>
Thanks! Merged branch to trunk on r1897895.

Backporting changes to 1.8.x in my TODO list.

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Evgeny Kotkov <ev...@visualsvn.com>]

Ivan Zhakov <iv...@visualsvn.com> writes:

> This part is now in the following branch:
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>
> What do you think?
>
> It would be great if someone could take a look on the implementation from
> the *nix perspective.
> After that, I propose to merge the branch into trunk.

In case this helps, I have tested the branch on Ubuntu x64 and it seems
to compile and pass the tests.

(While here, I noticed that APR trunk doesn't build on Windows;
 have posted on that separately.)


Thanks,
Evgeny Kotkov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Thu, 20 Jan 2022 at 17:39, Ivan Zhakov <iv...@visualsvn.com> wrote:

> On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <iv...@visualsvn.com> wrote:
>
>> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>>
>>>
>>> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
>>> > [[ sorry for delayed response ]]
>>> >
>>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>>> >>
>>> >> Hi Ivan,
>>> >>
>>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com>
>>> wrote:
>>> >>>
>>> >>> This change does not compile on Windows in APR 1.7.x:
>>> >>> [[[
>>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
>>> identifier
>>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand'
>>> must
>>> >>> point to struct/union
>>> >>
>>> >> I was missing backport of r1895178, does r1896808 compile now?
>>> >> (Sorry, no Windows at hand..).
>>> > Yes, it builds now. Thanks!
>>> >
>>> >>
>>> >>>
>>> >>> I also have a high-level objection against backporting this change to
>>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>>> >>> regression fixes should be backported to the stable branch. r1896510
>>> >>> is a significant change and as far as I understand it's not a
>>> >>> regression fix. So I think it would be better to revert r1896510 and
>>> >>> release it as part of APR 2.0 (or 1.8.x).
>>> >>
>>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>>> >> fixes for bugs that were there before 1.7 already, not regressions
>>> >> introduced by 1.7.0.
>>> >
>>> > Agreed on the bugfix/regressions part. I have misunderstood that
>>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
>>> > it adds new functionality. But even with that in mind, I still think
>>> > that the size of the change might be just too large for it to be an
>>> > appropriate fit for a patch release.
>>> >
>>> > Speaking of the change itself, I think that there might be an
>>> > alternative to making the apr_file_t also handle sockets on Windows.
>>> > It might be better to specifically change the pollset implementation
>>> > so that on Windows it would add a socket and use it for wakeup,
>>> > instead of using the socket disguised as a file.
>>> >
>>> > If this alternative approach sounds fine, I could try to implement it.
>>>
>>> But this could wait for a 1.7.2, correct? I am asking because there is
>>> some desire to get 1.7.1 out of the door soon.
>>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and
>>> is released soon after 1.7.2.
>>>
>>> 1. Revert this change from 1.7.x
>> 2. Release 1.7.1
>> 3. Rework this code on trunk without changing the apr_file_t's behavior
>>
> This part is now in the following branch:
>
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>
> Sorry, wrong branch URL. The correct URL is:
https://svn.apache.org/repos/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <iv...@visualsvn.com> wrote:

> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:
>
>>
>>
>> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
>> > [[ sorry for delayed response ]]
>> >
>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>> >>
>> >> Hi Ivan,
>> >>
>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>>
>> >>> This change does not compile on Windows in APR 1.7.x:
>> >>> [[[
>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
>> identifier
>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>> >>> point to struct/union
>> >>
>> >> I was missing backport of r1895178, does r1896808 compile now?
>> >> (Sorry, no Windows at hand..).
>> > Yes, it builds now. Thanks!
>> >
>> >>
>> >>>
>> >>> I also have a high-level objection against backporting this change to
>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>> >>> regression fixes should be backported to the stable branch. r1896510
>> >>> is a significant change and as far as I understand it's not a
>> >>> regression fix. So I think it would be better to revert r1896510 and
>> >>> release it as part of APR 2.0 (or 1.8.x).
>> >>
>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>> >> fixes for bugs that were there before 1.7 already, not regressions
>> >> introduced by 1.7.0.
>> >
>> > Agreed on the bugfix/regressions part. I have misunderstood that
>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
>> > it adds new functionality. But even with that in mind, I still think
>> > that the size of the change might be just too large for it to be an
>> > appropriate fit for a patch release.
>> >
>> > Speaking of the change itself, I think that there might be an
>> > alternative to making the apr_file_t also handle sockets on Windows.
>> > It might be better to specifically change the pollset implementation
>> > so that on Windows it would add a socket and use it for wakeup,
>> > instead of using the socket disguised as a file.
>> >
>> > If this alternative approach sounds fine, I could try to implement it.
>>
>> But this could wait for a 1.7.2, correct? I am asking because there is
>> some desire to get 1.7.1 out of the door soon.
>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is
>> released soon after 1.7.2.
>>
>> 1. Revert this change from 1.7.x
> 2. Release 1.7.1
> 3. Rework this code on trunk without changing the apr_file_t's behavior
>
This part is now in the following branch:
https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation

What do you think?

It would be great if someone could take a look on the implementation from
the *nix perspective.  After that, I propose to merge the branch into trunk.

(I will be travelling/on vacation for a couple of weeks, so might not be
able to answer promptly.)

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> 1. Revert this change from 1.7.x

Done in r1897222.

Regards;
Yann.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

On Fri, Jan 14, 2022 at 4:40 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >
> > 1. Revert this change from 1.7.x
> > 2. Release 1.7.1
> > 3. Rework this code on trunk without changing the apr_file_t's behavior
> > 4. Backport it to 1.7.x/1.8.x
> >
> > And if this plan makes sense, I am ready to proceed with steps (1), (3) and (4).
>
> +1, not sure we'd later release both 1.7.2 and 1.8.0 later on,
> probably only the latter.

Btw, I don't think the _behaviour_ of apr_file_t changed, the
implementation of apr_file_socket_pipe did but it should be
transparent for the user.

>
> Cheers;
> Yann.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> 1. Revert this change from 1.7.x
> 2. Release 1.7.1
> 3. Rework this code on trunk without changing the apr_file_t's behavior
> 4. Backport it to 1.7.x/1.8.x
>
> And if this plan makes sense, I am ready to proceed with steps (1), (3) and (4).

+1, not sure we'd later release both 1.7.2 and 1.8.0 later on,
probably only the latter.

Cheers;
Yann.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:

>
>
> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> > [[ sorry for delayed response ]]
> >
> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >>
> >> Hi Ivan,
> >>
> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>>
> >>> This change does not compile on Windows in APR 1.7.x:
> >>> [[[
> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
> identifier
> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> >>> point to struct/union
> >>
> >> I was missing backport of r1895178, does r1896808 compile now?
> >> (Sorry, no Windows at hand..).
> > Yes, it builds now. Thanks!
> >
> >>
> >>>
> >>> I also have a high-level objection against backporting this change to
> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> >>> regression fixes should be backported to the stable branch. r1896510
> >>> is a significant change and as far as I understand it's not a
> >>> regression fix. So I think it would be better to revert r1896510 and
> >>> release it as part of APR 2.0 (or 1.8.x).
> >>
> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> >> fixes for bugs that were there before 1.7 already, not regressions
> >> introduced by 1.7.0.
> >
> > Agreed on the bugfix/regressions part. I have misunderstood that
> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> > it adds new functionality. But even with that in mind, I still think
> > that the size of the change might be just too large for it to be an
> > appropriate fit for a patch release.
> >
> > Speaking of the change itself, I think that there might be an
> > alternative to making the apr_file_t also handle sockets on Windows.
> > It might be better to specifically change the pollset implementation
> > so that on Windows it would add a socket and use it for wakeup,
> > instead of using the socket disguised as a file.
> >
> > If this alternative approach sounds fine, I could try to implement it.
>
> But this could wait for a 1.7.2, correct? I am asking because there is
> some desire to get 1.7.1 out of the door soon.
> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is
> released soon after 1.7.2.
>
> 1. Revert this change from 1.7.x
2. Release 1.7.1
3. Rework this code on trunk without changing the apr_file_t's behavior
4. Backport it to 1.7.x/1.8.x

And if this plan makes sense, I am ready to proceed with steps (1), (3) and
(4).

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[William A Rowe Jr <wr...@rowe-clan.net>]

On Thu, Jan 13, 2022 at 2:37 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> >
> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >>
> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>>
> >>> I also have a high-level objection against backporting this change to
> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> >>> regression fixes should be backported to the stable branch. r1896510
> >>> is a significant change and as far as I understand it's not a
> >>> regression fix. So I think it would be better to revert r1896510 and
> >>> release it as part of APR 2.0 (or 1.8.x).
> >>
> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> >> fixes for bugs that were there before 1.7 already, not regressions
> >> introduced by 1.7.0.
> >
> > Agreed on the bugfix/regressions part. I have misunderstood that
> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> > it adds new functionality. But even with that in mind, I still think
> > that the size of the change might be just too large for it to be an
> > appropriate fit for a patch release.
> >
> > Speaking of the change itself, I think that there might be an
> > alternative to making the apr_file_t also handle sockets on Windows.
> > It might be better to specifically change the pollset implementation
> > so that on Windows it would add a socket and use it for wakeup,
> > instead of using the socket disguised as a file.
> >
> > If this alternative approach sounds fine, I could try to implement it.
>
> But this could wait for a 1.7.2, correct? I am asking because there is some desire to get 1.7.1 out of the door soon.
> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is released soon after 1.7.2.

Although this doesn't "appear" to introduce an API change, based on the
scope it seems significant enough to merit a 1.8.0 release, following
1.7.1. I've forked a working branch 1.8.x to continue this effort without
interruption. Your call whether it should persist in 1.7.1, it seems that
apr >= 1.8 is an easier decision point for developers looking for these
enhancements.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ruediger Pluem <rp...@apache.org>]

On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> [[ sorry for delayed response ]]
> 
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Hi Ivan,
>>
>> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>
>>> This change does not compile on Windows in APR 1.7.x:
>>> [[[
>>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
>>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>>> point to struct/union
>>
>> I was missing backport of r1895178, does r1896808 compile now?
>> (Sorry, no Windows at hand..).
> Yes, it builds now. Thanks!
> 
>>
>>>
>>> I also have a high-level objection against backporting this change to
>>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>>> regression fixes should be backported to the stable branch. r1896510
>>> is a significant change and as far as I understand it's not a
>>> regression fix. So I think it would be better to revert r1896510 and
>>> release it as part of APR 2.0 (or 1.8.x).
>>
>> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>> fixes for bugs that were there before 1.7 already, not regressions
>> introduced by 1.7.0.
> 
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality. But even with that in mind, I still think
> that the size of the change might be just too large for it to be an
> appropriate fit for a patch release.
> 
> Speaking of the change itself, I think that there might be an
> alternative to making the apr_file_t also handle sockets on Windows.
> It might be better to specifically change the pollset implementation
> so that on Windows it would add a socket and use it for wakeup,
> instead of using the socket disguised as a file.
> 
> If this alternative approach sounds fine, I could try to implement it.

But this could wait for a 1.7.2, correct? I am asking because there is some desire to get 1.7.1 out of the door soon.
And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is released soon after 1.7.2.

Regards

Rüdiger

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

Hi Ivan;

On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > I was missing backport of r1895178, does r1896808 compile now?
> > (Sorry, no Windows at hand..).
> Yes, it builds now. Thanks!

Great, thanks for testing.

> >
> > I think that most if not all of the changes to 1.7.x since 1.7.0 are
> > fixes for bugs that were there before 1.7 already, not regressions
> > introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality. But even with that in mind, I still think
> that the size of the change might be just too large for it to be an
> appropriate fit for a patch release.

The bugfix part is quite probable though still theoretical on the
httpd side. As you may know mpm_event makes heavy use of the wakeup
pipe with potentially plenty of worker threads waking up one listener
thread, and that should not block for obvious event scheme scalability
reasons. But there is actually no bug report about this since blocking
would not result in a deadlock and httpd users have not tested the new
implementation performance wise, yet. Theoretically it may happen..
So since mpm_event is unix(es) only, this change is mainly aimed at
fixing the unix implementation of the wakeup pipe (write a single byte
atomically/once until it's consumed), but that can't happen portably
on the APR side without adapting the Windows implementation given that
there is no synchronous/nonblocking ReadFile().
Using {Read,Write}File() on a SOCKET seems to be supported by Windows,
but if the SOCKET is set nonblocking it does not behave accordingly
(from what I could read on the subject), hence the switch to
WSA{Recv,Send}(). Note that there was already a special "pipe"
handling in Windows' apr_file_t, the "socket" one is just a third
case.
I don't find it too cumbersome personally, but if you have a better
option I'm all for it!

>
> Speaking of the change itself, I think that there might be an
> alternative to making the apr_file_t also handle sockets on Windows.
> It might be better to specifically change the pollset implementation
> so that on Windows it would add a socket and use it for wakeup,
> instead of using the socket disguised as a file.

AIUI, that's already what OS/2 does (using an UDP socket), having its
own apr_pollset_wakeup() implementation and its own draining directly
in apr_pollset_poll() instead of reusing the common
apr_poll_drain_wakeup_pipe(), which is the one performing the
apr_file_read() currently.
That's certainly an option for Windows too, and in that area it may be
interesting to look at an IOCB implementation for polling rather than
select (but that's probably another story given the different
semantics of IOCB vs epoll/kqueue..).

>
> If this alternative approach sounds fine, I could try to implement it.

Looks good to me, not sure it should block the current implementation
in 1.7.x though because AFAIK it passes the tests suite on Windows too
(which Mladen Turk made work and tried IIRC). The changes are not
trivial but quite straightforward given the previous/existing socket
as pipe implementation in apr_file_t.

But I don't want to push anything, mpm_event has worked like this for
quite some time now and it can probably wait for 1.8.x for this
supposed fix/improvement.


Regards;
Yann.

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > I think that most if not all of the changes to 1.7.x since 1.7.0 are
> > fixes for bugs that were there before 1.7 already, not regressions
> > introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality.

So after reverting r1896510, the reported bug in [1] is still
addressed by r1894916, but only on unixes then.

[1] https://lists.apache.org/thread/mgosgwpqpxqrhh1q06z9okqgfqr46q24

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ivan Zhakov <iv...@visualsvn.com>]

[[ sorry for delayed response ]]

On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>
> Hi Ivan,
>
> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >
> > This change does not compile on Windows in APR 1.7.x:
> > [[[
> > file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
> > file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> > point to struct/union
>
> I was missing backport of r1895178, does r1896808 compile now?
> (Sorry, no Windows at hand..).
Yes, it builds now. Thanks!

>
> >
> > I also have a high-level objection against backporting this change to
> > APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> > regression fixes should be backported to the stable branch. r1896510
> > is a significant change and as far as I understand it's not a
> > regression fix. So I think it would be better to revert r1896510 and
> > release it as part of APR 2.0 (or 1.8.x).
>
> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> fixes for bugs that were there before 1.7 already, not regressions
> introduced by 1.7.0.

Agreed on the bugfix/regressions part. I have misunderstood that
r1896510 is a bugfix, perhaps, due to its size, and was thinking that
it adds new functionality. But even with that in mind, I still think
that the size of the change might be just too large for it to be an
appropriate fit for a patch release.

Speaking of the change itself, I think that there might be an
alternative to making the apr_file_t also handle sockets on Windows.
It might be better to specifically change the pollset implementation
so that on Windows it would add a socket and use it for wakeup,
instead of using the socket disguised as a file.

If this alternative approach sounds fine, I could try to implement it.

-- 
Ivan Zhakov

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Ruediger Pluem <rp...@apache.org>]

On 1/7/22 3:32 PM, Yann Ylavic wrote:
> Hi Ivan,
> 
> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> This change does not compile on Windows in APR 1.7.x:
>> [[[
>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>> point to struct/union
> 
> I was missing backport of r1895178, does r1896808 compile now?
> (Sorry, no Windows at hand..).
> 
>>
>> I also have a high-level objection against backporting this change to
>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>> regression fixes should be backported to the stable branch. r1896510
>> is a significant change and as far as I understand it's not a
>> regression fix. So I think it would be better to revert r1896510 and
>> release it as part of APR 2.0 (or 1.8.x).
> 
> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> fixes for bugs that were there before 1.7 already, not regressions
> introduced by 1.7.0.

That was also my read of the versioning rules. We only cannot add new stuff. For this we need to bump the minor version.

Regards

Rüdiger

Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/

[Yann Ylavic <yl...@gmail.com>]

Hi Ivan,

On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> This change does not compile on Windows in APR 1.7.x:
> [[[
> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> point to struct/union

I was missing backport of r1895178, does r1896808 compile now?
(Sorry, no Windows at hand..).

>
> I also have a high-level objection against backporting this change to
> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> regression fixes should be backported to the stable branch. r1896510
> is a significant change and as far as I understand it's not a
> regression fix. So I think it would be better to revert r1896510 and
> release it as part of APR 2.0 (or 1.8.x).

I think that most if not all of the changes to 1.7.x since 1.7.0 are
fixes for bugs that were there before 1.7 already, not regressions
introduced by 1.7.0.

But I'm fine with switching to 1.8.x and releasing 1.8.0 if that's the
policy, not sure that 1.7.1 is needed then (and we'd need to revert
quite some changes in 1.7.x..).

Regards;
Yann.