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
From 1948fc273c4f4b78f90c8ca8fbc22647e30f431c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Chigot?= <chi...@adacore.com>
Date: Wed, 24 Aug 2022 15:42:33 +0200
Subject: [PATCH] io: switch completely to Windows API for socket watch on
win32
The current implementation is using a mixed of select and the Windows
API (WSASelectEvent, etc).
As select doesn't clear the Windows events, WSAENumNetworkEvents must be
called before select to do it.
However, as this operation isn't made atomically, some events might be
reset before being retrieved by select. Resulting in them being skipped
by the polling.
In order to avoid this issue, this patch improves socket watch to be
based solely on Windows API.
Note that because several GSources might be created for the same HANDLE,
this implementation has no way to know on which GSources an event must be
returned. However, it seems that only two GSources are created, a master
one with just "G_IO_HUP" and the child one which aims to retrieve all
the events (ie with "G_IO_IN", "G_IO_OUT", etc). So, to avoid the master
GSource receiving events meant for the child, we aren't calling
WSAEnumNetworkEvents for it (or for any GSource having only G_IO_HUP as
condition). As G_IO_HUP was never returned by the previous, I supposed
that it should be safe to do so.
TN: V812-020
Change-Id: I037fbe93e0641894b0096c39ace5d0d68db8e2e3
---
io/channel-socket.c | 2 +-
io/channel-watch.c | 57 +++++++++++++++++++++++----------------------
2 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/io/channel-socket.c b/io/channel-socket.c
index b76dca9cc1..75500e5647 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -69,7 +69,7 @@ qio_channel_socket_new(void)
qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
#ifdef WIN32
- ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL);
+ ioc->event = WSACreateEvent();
#endif
trace_qio_channel_socket_new(sioc);
diff --git a/io/channel-watch.c b/io/channel-watch.c
index 0289b3647c..9c9aca41f5 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -114,46 +114,39 @@ qio_channel_socket_source_prepare(GSource *source G_GNUC_UNUSED,
static gboolean
qio_channel_socket_source_check(GSource *source)
{
- static struct timeval tv0;
-
QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
WSANETWORKEVENTS ev;
- fd_set rfds, wfds, xfds;
if (!ssource->condition) {
return 0;
}
- WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
-
- FD_ZERO(&rfds);
- FD_ZERO(&wfds);
- FD_ZERO(&xfds);
- if (ssource->condition & G_IO_IN) {
- FD_SET((SOCKET)ssource->socket, &rfds);
- }
- if (ssource->condition & G_IO_OUT) {
- FD_SET((SOCKET)ssource->socket, &wfds);
- }
- if (ssource->condition & G_IO_PRI) {
- FD_SET((SOCKET)ssource->socket, &xfds);
+ /* For now, we don't support G_IO_HUP checks.
+ We want to avoid calling WSAEnumNetworkEvents for any GSource
+ having just G_IO_HUP. It might hide events aimed to be retrieved by
+ other GSources waiting inputs or outputs (ie with G_IO_IN or G_IO_OUT).
+ The reason is that the Windows API is based on HANDLE but we often
+ create several GSources for the same HANDLE. Thus, input events might
+ be picked and cleared by the G_IO_HUP GSource. */
+ if (ssource->condition == G_IO_HUP) {
+ return 0;
}
- ssource->revents = 0;
- if (select(0, &rfds, &wfds, &xfds, &tv0) == 0) {
+
+ if (WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev)) {
return 0;
}
- if (FD_ISSET(ssource->socket, &rfds)) {
+ ssource->revents = 0;
+
+ if (ev.lNetworkEvents & (FD_READ | FD_ACCEPT | FD_OOB)) {
ssource->revents |= G_IO_IN;
}
- if (FD_ISSET(ssource->socket, &wfds)) {
+
+ if (ev.lNetworkEvents & (FD_WRITE | FD_CONNECT)) {
ssource->revents |= G_IO_OUT;
}
- if (FD_ISSET(ssource->socket, &xfds)) {
- ssource->revents |= G_IO_PRI;
- }
- return ssource->revents;
+ return ssource->revents & ssource->condition;
}
@@ -174,6 +167,7 @@ qio_channel_socket_source_finalize(GSource *source)
{
QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
+
object_unref(OBJECT(ssource->ioc));
}
@@ -286,9 +280,16 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
QIOChannelSocketSource *ssource;
#ifdef WIN32
- WSAEventSelect(socket, ioc->event,
- FD_READ | FD_ACCEPT | FD_CLOSE |
- FD_CONNECT | FD_WRITE | FD_OOB);
+ int ev = 0;
+
+ if (condition & G_IO_IN) {
+ ev |= (FD_READ | FD_ACCEPT | FD_OOB);
+ }
+ if (condition & G_IO_OUT) {
+ ev |= (FD_WRITE | FD_CONNECT);
+ }
+
+ WSAEventSelect(socket, ioc->event, ev);
#endif
source = g_source_new(&qio_channel_socket_source_funcs,
@@ -303,7 +304,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
ssource->revents = 0;
ssource->fd.fd = (gintptr)ioc->event;
- ssource->fd.events = G_IO_IN;
+ ssource->fd.events = condition;
g_source_add_poll(source, &ssource->fd);
--
2.25.1
