Hi Ivan;
On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov <i...@visualsvn.com> wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <ylavic....@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.
