Re: [PATCH 49/51] io/channel-watch: Fix socket watch on Windows

[Bin Meng]

On Wed, Sep 7, 2022 at 1:07 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Clément,
>
> On Tue, Sep 6, 2022 at 8:06 PM Clément Chigot <chigot@adacore.com> wrote:
> >
> > > > > I checked your patch, what you did seems to be something one would
> > > > > naturally write, but what is currently in the QEMU sources seems to be
> > > > > written intentionally.
> > > > >
> > > > > +Paolo Bonzini , you are the one who implemented the socket watch on
> > > > > Windows. Could you please help analyze this issue?
> > > > >
> > > > > > to avoid WSAEnumNetworkEvents for the master GSource which only has
> > > > > > G_IO_HUP (or for any GSource having only that).
> > > > > > As I said above, the current code doesn't do anything with it 
> > > > > > anyway.
> > > > > > So, IMO, it's safe to do so.
> > > > > >
> > > > > > I'll send you my patch attached. I was planning to send it in the 
> > > > > > following
> > > > > > weeks anyway. I was just waiting to be sure everything looks fine 
> > > > > > on our
> > > > > > CI. Feel free to test and modify it if needed.
> > > > >
> > > > > I tested your patch. Unfortunately there is still one test case
> > > > > (migration-test.exe) throwing up the "Broken pipe" message.
> > > >
> > > > I must say I didn't fully test it against qemu testsuite yet. Maybe 
> > > > there are
> > > > some refinements to be done. "Broken pipe" might be linked to the 
> > > > missing
> > > > G_IO_HUP support.
> > > >
> > > > > Can you test my patch instead to see if your gdb issue can be fixed?
> > > >
> > > > Yeah sure. I'll try to do it this afternoon.
> >
> > I can't explain how mad at me I am... I'm pretty sure your patch was the 
> > first
> > thing I've tried when I encountered this issue. But it wasn't working
> > or IIRC the
> > issue went away but that was because the polling was actually disabled 
> > (looping
> > indefinitely)...I'm suspecting that I already had changed the CreateEvent 
> > for
> > WSACreateEvent which forces you to handle the reset.
> > Finally, I end up struggling reworking the whole check function...
> > But yeah, your patch does work fine on my gdb issues too.
>
> Good to know this patch works for you too.
>
> > And I guess the events are reset when recv() is being called because of the
> > auto-reset feature set up by CreateEvent().
> > IIUC, what Marc-André means by busy loop is the polling being looping
> > indefinitely as I encountered. I can ensure that this patch doesn't do that.
> > It can be easily checked by setting the env variable G_MAIN_POLL_DEBUG.
> > It'll show what g_poll is doing and it's normally always available on
> > Windows.
> >
> > Anyway, we'll wait for Paolo to see if he remembers why he had to call
> > WSAEnumNetworkEvents. Otherwize, let's go for your patch. Mine might
> > be a good start to improve the whole polling on Windows but if it doesn't
> > work in your case, it then needs some refinements.
> >
>
> Yeah, this issue bugged me quite a lot. If we want to reset the event
> in qio_channel_socket_source_check(), we will have to do the following
> to make sure qtests are happy.
>
> diff --git a/io/channel-watch.c b/io/channel-watch.c
> index 43d38494f7..f1e1650b81 100644
> --- a/io/channel-watch.c
> +++ b/io/channel-watch.c
> @@ -124,8 +124,6 @@ qio_channel_socket_source_check(GSource *source)
> return 0;
> }
> - WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
> -
> FD_ZERO(&rfds);
> FD_ZERO(&wfds);
> FD_ZERO(&xfds);
> @@ -153,6 +151,10 @@ qio_channel_socket_source_check(GSource *source)
> ssource->revents |= G_IO_PRI;
> }
> + if (ssource->revents) {
> + WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
> + }
> +
> return ssource->revents;
> }
>
> Removing "if (ssource->revents)" won't work.
>
> It seems to me that resetting the event twice (one time with the
> master Gsource, and the other time with the child GSource) causes some
> bizarre behavior. But MSDN [1] says
>
>     "Resetting an event that is already reset has no effect."
>
> [1] 
> https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-resetevent
>

Paolo, any comments about this issue?

Regards,
Bin