Re: [PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
On Tue, Jul 09, 2013 at 06:58:32PM +0100, Neil Roberts wrote:
> Bill Spitzak writes:
>
> > Maybe it can abort if ...read_events() is called after an error was
> > encountered. Then bad toolkits will exit, while ones that check for the
> > error state can do something intelligent.
>
> This sounds like a nice idea but it might go a bit wrong if there are
> orthogonal bits of code reading the events. For example if the error is
> first detected in eglSwapBuffers() then the first time the application
> will know about it is when it later calls wl_display_dispatch, but that
> will already be the second thing that tries to read the error so it
> would abort.
>
> That actually highlights a more serious problem. As soon as the error is
> encountered libwayland-client will actually close the socket. That means
> that eglSwapBuffers might cause the socket to close without the
> application having a chance to know about it. It will then try to poll
> on an invalid socket. Or worse still, something might try to open a file
> in the interim and end up reusing the file descriptor. Then it would
> actually poll on a random unrelated file and completely miss the error!
Argh yes, it's bad to close the fd in display_fatal_error(), we need
to keep it open until the application destroys the wl_display. I was
thinking that maybe that's also why we're not catching POLLHUP from
poll, but it turns out it's much sillier than that:
commit 99b78fdd7f21539a288bfe846542b633756ce163
Author: Kristian Høgsberg
Date: Tue Jul 9 18:35:06 2013 -0400
wayland: Don't clear revents until we've checked for G_IO_HUP
https://bugzilla.gnome.org/show_bug.cgi?id=703892
diff --git a/gdk/wayland/gdkeventsource.c b/gdk/wayland/gdkeventsource.c
index cb335bd..4bcae9b 100644
--- a/gdk/wayland/gdkeventsource.c
+++ b/gdk/wayland/gdkeventsource.c
@@ -150,12 +150,12 @@ _gdk_wayland_display_queue_events (GdkDisplay *display)
display_wayland = GDK_WAYLAND_DISPLAY (display);
source = (GdkWaylandEventSource *) display_wayland->event_source;
+
if (source->pfd.revents & G_IO_IN)
-{
- wl_display_dispatch(display_wayland->wl_display);
- source->pfd.revents = 0;
-}
+wl_display_dispatch (display_wayland->wl_display);
if (source->pfd.revents & (G_IO_ERR | G_IO_HUP))
g_error ("Lost connection to wayland compositor");
+
+ source->pfd.revents = 0;
}
but your patches (wayland and gtk+) still make sense of course. I
pushed your wayland patch with Robs wording tweak and my follow-up fix
to not return -1 when we get EAGAIN, and I pushed both your gtk+ fix
and the above gtk+ fix. And both gtk+ fixes are necessary - we can
get G_IO_IN | G_IO_HUP, in which case wl_display_dispatch() will read
whatever data we got up until the HUP and return 0 and the G_IO_HUP
test will the catch the error. Or we may get another error from
wl_display_dispatch() that isn't reflected in the revents flags.
Kristian
> Egads,
> - Neil
> -
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
On Tue, Jul 09, 2013 at 02:10:45PM +0100, Neil Roberts wrote:
> If EOF is encountered while reading from the Wayland socket,
> wl_display_read_events() will now return -1 so that it will be treated
> as an error. The documentation for this function states that it will
> set errno when there is an error so it additionally makes up an errno
> of EPIPE.
Ah, yes, good catch. The multi-thread behaviour of
wl_display_read_events() allows mulitple threads to call into it, but
only one of the threads will get to read the data from the socket.
When I wrote this I was thinking that all other threads will call
recvmsg non-blocking and return 0 bytes. But 0 means "orderly
shutdown" of course, while -1 and errno = EAGAIN is "no data right
now". We do handle that case, but return -1, which the higher-level
code will treat as an error. I'll fix that in a follow-up commit.
> If we don't do this then when the compositor quits the Wayland socket
> will be become ready for reading but wl_display_dispatch will do
> nothing which typically makes the application take up 100% CPU. In
> particular eglSwapBuffers will likely get stuck in an infinite busy
> loop because it repeatedly calls wl_display_dispatch_queue while it
> waits for the frame callback.
Yes... I wonder why gtk+ doesn't handle POLLHUP and quits from that
though.
> ---
> src/wayland-client.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index cb091ab..9400bcd 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -877,6 +877,15 @@ read_events(struct wl_display *display)
> display_fatal_error(display, errno);
> return -1;
> }
> + else if (total == 0) {
> + /* The compositor has closed the socket. This
> + * should be considered an error so we'll fake
> + * an errno */
> + errno = EPIPE;
Yeah, EPIPE is fine I suppose.
thanks,
Kristian
> + display_fatal_error(display, errno);
> + return -1;
> + }
> +
> for (rem = total; rem >= 8; rem -= size) {
> size = queue_event(display, rem);
> if (size == -1) {
> --
> 1.7.11.3.g3c3efa5
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
Bill Spitzak writes: > Maybe it can abort if ...read_events() is called after an error was > encountered. Then bad toolkits will exit, while ones that check for the > error state can do something intelligent. This sounds like a nice idea but it might go a bit wrong if there are orthogonal bits of code reading the events. For example if the error is first detected in eglSwapBuffers() then the first time the application will know about it is when it later calls wl_display_dispatch, but that will already be the second thing that tries to read the error so it would abort. That actually highlights a more serious problem. As soon as the error is encountered libwayland-client will actually close the socket. That means that eglSwapBuffers might cause the socket to close without the application having a chance to know about it. It will then try to poll on an invalid socket. Or worse still, something might try to open a file in the interim and end up reusing the file descriptor. Then it would actually poll on a random unrelated file and completely miss the error! Egads, - Neil - Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
> the only thing that confused me was the past tense sentence […]
Ok, that sounds sensible. Here is an updated patch.
> However we'd have to change every client / toolkit and so using a
> fatal display error is nicer since the toolkits should already be
> handling that.
Well, that would be ideal but it looks like actually GTK and
Clutter/Cogl don't currently handle any errors so they will both end
up constantly waking up the main loop and hitting 100% CPU. Maybe we
should make them both abort on error like what would happen if you
were using Xlib by default.
Regards,
- Neil
-- >8 --
If EOF is encountered while reading from the Wayland socket, make
wl_display_read_events() return -1 so that it will be treated as an
error. The documentation for this function states that it will set
errno when there is an error so it additionally makes up an errno of
EPIPE.
If we don't do this then when the compositor quits the Wayland socket
will be become ready for reading but wl_display_dispatch will do
nothing which typically makes the application take up 100% CPU. In
particular eglSwapBuffers will likely get stuck in an infinite busy
loop because it repeatedly calls wl_display_dispatch_queue while it
waits for the frame callback.
---
src/wayland-client.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index cb091ab..9400bcd 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -877,6 +877,15 @@ read_events(struct wl_display *display)
display_fatal_error(display, errno);
return -1;
}
+ else if (total == 0) {
+ /* The compositor has closed the socket. This
+* should be considered an error so we'll fake
+* an errno */
+ errno = EPIPE;
+ display_fatal_error(display, errno);
+ return -1;
+ }
+
for (rem = total; rem >= 8; rem -= size) {
size = queue_event(display, rem);
if (size == -1) {
--
1.7.11.3.g3c3efa5
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
Hi Neil, On 9 July 2013 14:10, Neil Roberts wrote: > If EOF is encountered while reading from the Wayland socket, > wl_display_read_events() will now return -1 so that it will be treated > as an error. The documentation for this function states that it will > set errno when there is an error so it additionally makes up an errno > of EPIPE. > > If we don't do this then when the compositor quits the Wayland socket > will be become ready for reading but wl_display_dispatch will do > nothing which typically makes the application take up 100% CPU. In > particular eglSwapBuffers will likely get stuck in an infinite busy > loop because it repeatedly calls wl_display_dispatch_queue while it > waits for the frame callback. Good discovery - the only thing that confused me was the past tense sentence: "If EOF is encountered while reading from the Wayland socket, wl_display_read_events() will now return -1 so that it will be treated as an error." It sound like you're reacting to some changed behaviour rather than changing behaviour. I propose "If EOF is encountered while reading from the Wayland socket, make wl_display_read_events() -1 so that it will be treated as an error" It does feel like a slight hack and instead we should some how propagate the EOF/zero to read up rather than synthesizing an error. However we'd have to change every client / toolkit and so using a fatal display error is nicer since the toolkits should already be handling that. Rob ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] wayland-client: Treat EOF when reading the wayland socket as an error
If EOF is encountered while reading from the Wayland socket,
wl_display_read_events() will now return -1 so that it will be treated
as an error. The documentation for this function states that it will
set errno when there is an error so it additionally makes up an errno
of EPIPE.
If we don't do this then when the compositor quits the Wayland socket
will be become ready for reading but wl_display_dispatch will do
nothing which typically makes the application take up 100% CPU. In
particular eglSwapBuffers will likely get stuck in an infinite busy
loop because it repeatedly calls wl_display_dispatch_queue while it
waits for the frame callback.
---
src/wayland-client.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index cb091ab..9400bcd 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -877,6 +877,15 @@ read_events(struct wl_display *display)
display_fatal_error(display, errno);
return -1;
}
+ else if (total == 0) {
+ /* The compositor has closed the socket. This
+* should be considered an error so we'll fake
+* an errno */
+ errno = EPIPE;
+ display_fatal_error(display, errno);
+ return -1;
+ }
+
for (rem = total; rem >= 8; rem -= size) {
size = queue_event(display, rem);
if (size == -1) {
--
1.7.11.3.g3c3efa5
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
