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

[Clément Chigot]

Hi all,

I did reach the same issue while trying to connect a gdb to qemu on
Windows hosts. Some inputs send by gdb aren't getting correctly pulled,
they will be retrieved only once g_poll times out.

As you explained earlier, the issue, AFAICT, is that WSAEnumNetworkEvents
will reset the internal events before select can detect them.
Sadly, I didn't find any way to adjust the current code using select to
avoid that. As select and any cleaner (ever WSAEnumNetworkEvents or
WSEResetEvent) cannot be called in an atomic way, it seems that events
can always be received between the reset and the select. At least, all
my attempts failed.

The only solution I've found is to move completely to the Windows API
and stop calling select. This, however, needs some tricks. In Particular, we
need to "remove" the support of GSources having only G_IO_HUP.
This is already kind of done as we currently don't detect them in
qio_channel_socket_source_check anyway.
This is mandatory because of the two GSources created on the same socket.
IIRC, the first one (I'll call it the master GSource) is created during
the initialization of the channel-socket by update_ioc_handlers and will
have only G_IO_HUP to catch up.
The second one is created during the "prepare" phase of the first one,
in io_watch_poll_prepare. This one will have all the events we want
to pull (G_IO_IN here).
As they are referring to the same socket, the Windows API cannot know
on which GSources we want to catch which events. The solution is then
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.

PS: I don't know if it will correctly extend if I simply attach it to
my mail. If not, tell me I'll simply copy-paste it, even if it might
mess up the space/tab stuff.

> >> >>
> >> >> ---
> >> >> During the testing, I removed the following codes in 
> >> >> update_ioc_handlers():
> >> >>
> >> >>     remove_hup_source(s);
> >> >>     s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
> >> >>     g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
> >> >>                           chr, NULL);
> >> >>     g_source_attach(s->hup_source, chr->gcontext);
> >> >>
> >> >> and such change also makes the symptom go away.
> >> >>
> >> >> And if I moved the above codes to the beginning, before the call to
> >> >> io_add_watch_poll(), the symptom also goes away.
> >> >>
> >> >> It seems two sources watching on the same socket event object is
> >> >> the key that leads to the instability. The order of adding a source
> >> >> watch seems to also play a role but I can't explain why.
> >> >> Hopefully a Windows and glib expert could explain this behavior.
> >> >>
> >> >
> >> > Feel free to leave that comment in the commit message.
> >>
> >> Sure, will add the above message into the commit in v2.
> >>
> >> >
> >> > This is strange, as both sources should have different events, clearing 
> >> > one shouldn't affect the other.
> >>
> >> Both sources have the same event, as one QIO channel only has one
> >> socket, and one socket can only be bound to one event.
> >
> >
> >  "one socket can only be bound to one event", is that a WSAEventSelect 
> > limitation?
> >
>
> Yes, please see the MSDN:
>
> It is not possible to specify different event objects for different
> network events. The following code will not work; the second call will
> cancel the effects of the first, and only the FD_WRITE network event
> will be associated with hEventObject2:
>
> rc = WSAEventSelect(s, hEventObject1, FD_READ);
> rc = WSAEventSelect(s, hEventObject2, FD_WRITE); //bad

Yes, the Windows API is handled at socket levels. That's why having
two GSources on the same sockets is problematic.
Note that maybe there is a mix to be done between your patch with
the update_ioc_handlers part to be removed and my patch which improves
the polling of channel-watch.

Thanks,
Clément

0001-io-switch-completely-to-Windows-API-for-socket-watch.patch
Description: Text Data