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

[Bin Meng]

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

Regards,
Bin