[PATCH] client: Allow send error recovery without an abort

On Tue, 5 Jun 2018 18:14:54 -0700

Post by Lloyd Pique
Introduce a new call wl_display_set_error_handler_client(), which allows
a client to register a callback function which is invoked if there is an
error (possibly transient) while sending messages to the server.
This allows a Wayland client that is actually a nested server to try and
recover more gracefully from send errors, allowing it one to disconnect
and reconnect to the server if necessary.
The handler is called with the wl_display and the errno, and is expected
to return one of WL_SEND_ERROR_ABORT, WL_SEND_ERROR_FATAL, or
WL_SEND_ERROR_RETRY. The core-client code will then respectively abort()
the process (the default if no handler is set), set the display context
into an error state, or retry the send operation that failed.
The existing display test is extended to exercise the new function.

Post by Lloyd Pique
---
COPYING | 1 +
src/wayland-client-core.h | 75 +++++++++++++++++
src/wayland-client.c | 67 +++++++++++++++-
tests/display-test.c | 165 +++++++++++++++++++++++++++++++++++++-
4 files changed, 306 insertions(+), 2 deletions(-)
diff --git a/COPYING b/COPYING
index eb25a4e..bace5d7 100644
--- a/COPYING
+++ b/COPYING
@@ -2,6 +2,7 @@ Copyright Â© 2008-2012 Kristian HÃžgsberg
Copyright Â© 2010-2012 Intel Corporation
Copyright Â© 2011 Benjamin Franzke
Copyright Â© 2012 Collabora, Ltd.
+Copyright Â© 2018 Google LLC
Permission is hereby granted, free of charge, to any person obtaining a
copy of this software and associated documentation files (the "Software"),
diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
index 03e781b..a3e4b8e 100644
--- a/src/wayland-client-core.h
+++ b/src/wayland-client-core.h
@@ -257,6 +257,81 @@ wl_display_cancel_read(struct wl_display *display);
int
wl_display_read_events(struct wl_display *display);
+/** \enum wl_send_error_strategy
+ *
+ * This enum are the valid values that can be returned from a
+ * wl_send_error_handler_func_t
+ *
+ * \sa wl_send_error_handler_func_t
+ */
+enum wl_send_error_strategy {
+ /** Abort the client */
+ WL_SEND_ERROR_ABORT,
+ /** Put the display into a fatal error state */
+ WL_SEND_ERROR_FATAL,
+ /** Retry the send operation */
+ WL_SEND_ERROR_RETRY,
+};
+
+/**
+ * \brief A function pointer type for customizing client send error handling
+ *
+ * A client send error strategy handler is a callback function which the client
+ * can set to direct the core client library how to respond if an error is
+ * encountered while sending a message.
+ *
+ * Normally messages are enqueued to an output buffer by the core client
+ * library, and the user of the core client library is expected to call
+ * wl_display_flush() occasionally, but as that is non-blocking, messages can
+ * queue up in the output buffer. If later calls to send messages happen to
+ * fill up the output buffer, the core client library will make an internal
+ * call to flush the output buffer. But if the write call unexpectedly fails,
+ * as there is no good way to return an error to the caller, the core client
+ * library will log a message and call abort().

The analysis of the current situation is correct.

If you know you are sending lots of requests, you should be calling
wl_display_flush() occasionally. It will return -1 if flushing fails to
let you know you need to wait. The main problem I see with that is when
to attempt to flush.

I remember some discussion from the past, where we would make the
soft-buffer in libwayland-client larger and introduce a check API which
would return whether, say, more than half of the space is already
consumed. That would work as a trigger for an extra flush at a point
convenient to the client.

I remember discussing something like that with Daniel Stone, but I
can't find it. Might have been in IRC.

Or maybe it was more like, double the soft-buffer size, attempt an
automatic flush if it is more than half-full, and if that fails,
continue and raise a flag that the user can query at a convenient time
and do the dance for manual flush and poll for writable. The remaining
soft-buffer should hopefully allow the manual flush before the buffer
runs out.

Post by Lloyd Pique
+ *
+ * If instead a send error strategy function is set, the core client library
+ * will call it to allow the client to choose a different strategy.
+ *
+ * The function takes two arguments. The first is the display context for the
+ * error. The second is the errno for the failure that occurred.
+ *
+ * The handler is expected to return one of the wl_send_error_strategy values
+ * to indicate how the core client library should proceed.

This is the detail that bothers me: making the callback to determine
policy. You have no idea in what context that callback will be made.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_ABORT, the core client code will
+ * perform the default action -- log an error and then call the C runtime
+ * abort() function.

This is the old behaviour, which we agree is not nice.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_FATAL, the core client code will
+ * continue, but the display context will be put into a fatal error state, and
+ * the display should no longer be used. Further calls to send messages
+ * will function, but the message will not actually be enqueued.

This is the behaviour I would prefer for unrecoverable errors. I
actually wrote that down recently:
https://gitlab.freedesktop.org/wayland/wayland/issues/12

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_RETRY, the send operation is retried
+ * immediately on return, and may fail again. If it does, the handler will be
+ * called again with the new errno. To avoid a busy loop, the handler should
+ * wait on the display connection, or something equivalent. If after returning
+ * WL_CLIENT_ERROR_RETRY the send operation succeeds, the handler is called one
+ * last time with an errno of zero. This allows the handler to perform any
+ * necessary cleanup now that the send operation has succeeded. For this final
+ * call, the return value is ignored.
+ *
+ * Note that if the handler blocks, the current thread that is making the call
+ * to send a message will of course be blocked. This means the default behavior
+ * of Wayland client calls being non-blocking will no longer be true.
+ *
+ * In a multi threaded environment, this error handler could be called once for
+ * every thread that attempts to send a message on the display and encounters
+ * the error with the connection. It is the responsibility of the handler
+ * function be itself thread safe.

This third case is the red flag for me.

How could someone use this efficiently in real life? Do you have
example users for this?

How does the callback know at what kind of point of execution you are
in your application? Can you block there? Can you deal with
disconnection? What if the trigger fires inside a library you use, like
libEGL? Which holds mutexes of its own? What if it happens in a thread
you didn't expect, e.g. from GStreamer?

You'd probably need to maintain global flag variables across code
blocks. If you maintain something across code blocks, would it not be
easier instead to try a manual flush at the end of a block and see if
that works?

If flushing is inefficient, we could look into adding ABI to get
information about the buffer status, so you can make a better choice on
whether to flush or not.

If you attempt a manual flush and it fails, you will have more freedom
to handle that situation than with the callback you propose. You can do
everything you could with the callback and you could also choose to
return from the request sending code block and come back later, e.g. on
the next round of your main event loop or when then Wayland fd signals
writable in poll() again.

Interrupting what you are doing is much easier if you can pick the
points where it is possible. It is hard to be able to handle a
potential callback every time you call a request sending function.

Especially if the client is a nested server, you probably would not
want to block on the Wayland connection because that could prevent you
from servicing your own clients.

Do you think the alternative solutions could be workable for you?

I would be very happy to see someone working on those, this issue has
existed since the beginning.

Thanks,
pq

Post by Lloyd Pique
+ */
+typedef enum wl_send_error_strategy
+ (*wl_send_error_handler_func_t)(struct wl_display *, int /* errno */);
+
+void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t handler);
+
void
wl_log_set_handler_client(wl_log_func_t handler);
diff --git a/src/wayland-client.c b/src/wayland-client.c
index efeb745..3a9db6f 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -113,6 +113,8 @@ struct wl_display {
int reader_count;
uint32_t read_serial;
pthread_cond_t reader_cond;
+
+ wl_send_error_handler_func_t send_error_handler;
};
/** \endcond */
@@ -696,6 +698,44 @@ wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
proxy->version);
}
+static enum wl_send_error_strategy
+display_send_error_handler_invoke(struct wl_display *display, int send_errno)
+{
+ enum wl_send_error_strategy ret = WL_SEND_ERROR_ABORT;
+ wl_send_error_handler_func_t handler = display->send_error_handler;
+ if (handler) {
+ pthread_mutex_unlock(&display->mutex);
+ ret = handler(display, send_errno);
+ pthread_mutex_lock(&display->mutex);
+ }
+ return ret;
+}
+
+static void
+display_closure_send_error(struct wl_display *display,
+ struct wl_closure *closure)
+{
+ while (true) {
+ int send_errno = errno;
+ switch (display_send_error_handler_invoke(display, send_errno)) {
+ display_fatal_error(display, send_errno);
+ return;
+
+ if (!wl_closure_send(closure, display->connection)) {
+ display_send_error_handler_invoke(display, 0);
+ return;
+ }
+ break;
+
+ wl_abort("Error sending request: %s\n",
+ strerror(send_errno));
+ }
+ }
+}
/** Prepare a request to be sent to the compositor
*
@@ -743,6 +783,9 @@ wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
goto err_unlock;
}
+ if (proxy->display->last_error)
+ goto err_unlock;
+
closure = wl_closure_marshal(&proxy->object, opcode, args, message);
if (closure == NULL)
wl_abort("Error marshalling request: %s\n", strerror(errno));
@@ -751,7 +794,7 @@ wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,
wl_closure_print(closure, &proxy->object, true);
if (wl_closure_send(closure, proxy->display->connection))
- wl_abort("Error sending request: %s\n", strerror(errno));
+ display_closure_send_error(proxy->display, closure);
wl_closure_destroy(closure);
@@ -2000,6 +2043,28 @@ wl_display_flush(struct wl_display *display)
return ret;
}
+/** Sets the error handler to invoke when a send fails internally
+*
+* \param display The display context object
+* \param handler The function to call when an error occurs.
+*
+* Sets the error handler to call to decide what should happen on a send error.
+*
+* If no handler is set, the client will use the WL_SEND_ERROR_ABORT strategy.
+* This means the core client code will call abort() if if encounters a failure
+* while sending a message to the server.
+*
+* Setting a handler allows a client implementation to alter this behavior.
+*
+* \memberof wl_display
+*/
+WL_EXPORT void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t handler)
+{
+ display->send_error_handler = handler;
+}
+
/** Set the user data associated with a proxy
*
* \param proxy The proxy object

Lloyd Pique

2018-06-19 01:41:28 UTC

Hi Pekka!

Thank you for the feedback. In the end, I think we have a good basic
agreement about the patch, and after reading your linked bug (
https://gitlab.freedesktop.org/wayland/wayland/issues/12), which I was
previously unaware of, it sounds like I will be just stripping out the
callback+retry case from my patch.

I will probably go ahead and add a function to check for the half-full
output buffer, and one you didn't think of to check whether MAX_FDS_OUT is
about to be exceeded by the next call (that count is small enough to
actually be a concern).

At this point I think the only real question I have is one of checking the
intent behind the bug you filed. Should the design be that ONLY behavior
is to set a fatal error on the display context, or should there be a public
API to select between the previous/default abort and the detal error?

I'll send out a revised patch after hearing back with that bit of guidance.

In case it matters, I did also include my responses to your points inline
below. I don't expect to convince that this patch should be used as is, but
perhaps it will give you or someone else some clarify and insight into why
I suggested the change I did.

Thanks!

- Lloyd

Post by Pekka Paalanen
On Tue, 5 Jun 2018 18:14:54 -0700

Post by Lloyd Pique
Introduce a new call wl_display_set_error_handler_client(), which allows
a client to register a callback function which is invoked if there is an
error (possibly transient) while sending messages to the server.
This allows a Wayland client that is actually a nested server to try and
recover more gracefully from send errors, allowing it one to disconnect
and reconnect to the server if necessary.
The handler is called with the wl_display and the errno, and is expected
to return one of WL_SEND_ERROR_ABORT, WL_SEND_ERROR_FATAL, or
WL_SEND_ERROR_RETRY. The core-client code will then respectively abort()
the process (the default if no handler is set), set the display context
into an error state, or retry the send operation that failed.7
The existing display test is extended to exercise the new function.

"Software"),

Post by Lloyd Pique
diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
index 03e781b..a3e4b8e 100644
--- a/src/wayland-client-core.h
+++ b/src/wayland-client-core.h
@@ -257,6 +257,81 @@ wl_display_cancel_read(struct wl_display *display);
int
wl_display_read_events(struct wl_display *display);
+/** \enum wl_send_error_strategy
+ *
+ * This enum are the valid values that can be returned from a
+ * wl_send_error_handler_func_t
+ *
+ * \sa wl_send_error_handler_func_t
+ */
+enum wl_send_error_strategy {
+ /** Abort the client */
+ WL_SEND_ERROR_ABORT,
+ /** Put the display into a fatal error state */
+ WL_SEND_ERROR_FATAL,
+ /** Retry the send operation */
+ WL_SEND_ERROR_RETRY,
+};
+
+/**
+ * \brief A function pointer type for customizing client send error

handling

Post by Lloyd Pique
+ *
+ * A client send error strategy handler is a callback function which

the client

Post by Lloyd Pique
+ * can set to direct the core client library how to respond if an error

Post by Lloyd Pique
+ * encountered while sending a message.
+ *
+ * Normally messages are enqueued to an output buffer by the core client
+ * library, and the user of the core client library is expected to call
+ * wl_display_flush() occasionally, but as that is non-blocking,

messages can

Post by Lloyd Pique
+ * queue up in the output buffer. If later calls to send messages

happen to

Post by Lloyd Pique
+ * fill up the output buffer, the core client library will make an

internal

Post by Lloyd Pique
+ * call to flush the output buffer. But if the write call unexpectedly

fails,

Post by Lloyd Pique
+ * as there is no good way to return an error to the caller, the core

client

Post by Lloyd Pique
+ * library will log a message and call abort().

We actually do have a call to wl_display_flush() in our larger
implementation, called frequently as part of ensuring everything is
committed to the display. It does currently handle a return of -1, but by
waking up the input event processing thread in case the error is fatal, as
that thread handles disconnecting and reconnecting.

On the surface, making it detect an EAGAIN and wait kind of sounds
reasonable. But I kind of wonder how often in normal use it would return
EAGAIN, and so would force a wait that wouldn't need to, not when there is
still some space in the core client output buffer still. My reason for
adding the callback and the retry option were specifically to only make the
client block if there really was no other way to avoid an error state.

That said, I actually am willing to detect EAGAIN on wl_display_flush(),
and explicitly wait then. In trying out this patch, I only ended up
artificially reproducing an EAGAIN error (taking a few seconds to trigger)
when I magnified the request traffic by a factor of about 1000x. That is a
pretty unreasonable magnification even for a nested server, so while it
could happen, I don't think this is actually that important to my project.

In our case, the Wayland server we connect to is actually a small part of a
much larger Chrome UI process on ChromeOS. Despite being a user-space
process, it is not likely to be delayed in its processing of client
requests, and it seemed much more likely that the connection was dropping
for other reasons. Unfortunately I have no data on correlating our
anonymous crash reports (call stack only) with any data on why the Chrome
processes might have restarted (update? crash?). At least a clean reconnect
is much better than a wl_abort() for us, though still not a nice user
experience.

Post by Pekka Paalanen
I remember some discussion from the past, where we would make the
soft-buffer in libwayland-client larger and introduce a check API which
would return whether, say, more than half of the space is already
consumed. That would work as a trigger for an extra flush at a point
convenient to the client.
I remember discussing something like that with Daniel Stone, but I
can't find it. Might have been in IRC.
Or maybe it was more like, double the soft-buffer size, attempt an
automatic flush if it is more than half-full, and if that fails,
continue and raise a flag that the user can query at a convenient time
and do the dance for manual flush and poll for writable. The remaining
soft-buffer should hopefully allow the manual flush before the buffer
runs out.

I believe I saw the exchange at one point in an IRC archive while looking
for existing known problems, though I though I remember seeing it for the
receive buffers.

Thinking about it fresh, Maybe that is reasonable as a general option for
clients. The one caveat I could think of has to do with MAX_FDS_OUT and it
being only 28. My project does use the linux-dmabuf-unstable-v1 protocol,
and having more than 14 surfaces in a nested server seems like it could
actually happen often. And on second thought, I could see my client filling
up all 28 in one frame, thanks to perhaps an ill-timed flood of
notifications in addition to normal UI surfaces.

Hmm, the patch I've uploaded does catch that, but if I remove the RETRY
option I'll think I will need to add another special case
wl_display_flush() check when creating buffers. In my case I would actually
prefer to find out that the fd buffer is completely full before
waiting/flushing though.

Post by Lloyd Pique
+ *
+ * If instead a send error strategy function is set, the core client

library

Post by Lloyd Pique
+ * will call it to allow the client to choose a different strategy.
+ *
+ * The function takes two arguments. The first is the display context

for the

Post by Lloyd Pique
+ * error. The second is the errno for the failure that occurred.
+ *
+ * The handler is expected to return one of the wl_send_error_strategy

values

Post by Lloyd Pique
+ * to indicate how the core client library should proceed.

This is the detail that bothers me: making the callback to determine
policy. You have no idea in what context that callback will be made.

Sure, but I meant this to be a choice available to the author of the larger
client, assuming they had full knowledge of what the consequences of doing
anything non-trivial were.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_ABORT, the core client code

will

Post by Lloyd Pique
+ * perform the default action -- log an error and then call the C

runtime

Post by Lloyd Pique
+ * abort() function.

This is the old behaviour, which we agree is not nice.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_FATAL, the core client code

will

Post by Lloyd Pique
+ * continue, but the display context will be put into a fatal error

state, and

Post by Lloyd Pique
+ * the display should no longer be used. Further calls to send messages
+ * will function, but the message will not actually be enqueued.

This is the behaviour I would prefer for unrecoverable errors. I
https://gitlab.freedesktop.org/wayland/wayland/issues/12

I hadn't seen that bug before now, and it exactly matches the conditions I
observed. Hence my above response that I will go ahead and strip down my
change to something simpler.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_RETRY, the send operation is

retried

Post by Lloyd Pique
+ * immediately on return, and may fail again. If it does, the handler

will be

Post by Lloyd Pique
+ * called again with the new errno. To avoid a busy loop, the handler

should

Post by Lloyd Pique
+ * wait on the display connection, or something equivalent. If after

returning

Post by Lloyd Pique
+ * WL_CLIENT_ERROR_RETRY the send operation succeeds, the handler is

called one

Post by Lloyd Pique
+ * last time with an errno of zero. This allows the handler to perform

any

Post by Lloyd Pique
+ * necessary cleanup now that the send operation has succeeded. For

this final

Post by Lloyd Pique
+ * call, the return value is ignored.
+ *
+ * Note that if the handler blocks, the current thread that is making

the call

Post by Lloyd Pique
+ * to send a message will of course be blocked. This means the default

behavior

Post by Lloyd Pique
+ * of Wayland client calls being non-blocking will no longer be true.
+ *
+ * In a multi threaded environment, this error handler could be called

once for

Post by Lloyd Pique
+ * every thread that attempts to send a message on the display and

encounters

Post by Lloyd Pique
+ * the error with the connection. It is the responsibility of the

handler

Post by Lloyd Pique
+ * function be itself thread safe.

This third case is the red flag for me.
How could someone use this efficiently in real life? Do you have
example users for this?

Unfortunately that code is not open source, otherwise I would point you at
it.

My actual use case is a self-contained process that happens to use Wayland
for input and output, and that the use of Wayland there is an internal
implementation detail. While my client is a nested server, it doesn't
expose a Wayland interface to its clients -- I had to support another
interface entirely.

To me there only seems to be one real general pattern for the callback
handler when might implement the retry strategy. It is just slightly more
complex than the retry_with_poll_handler() from the test I added.

To sketch it out in quick pseudo-code, it would be:

int typical_retry_handler(struct wl_display *display, int send_errno) {

if (send_errno == 0) { /* retry succeeded */

/* clear retry start timestamp */

}

if (send_errno != EAGAIN) return WL_SEND_ERROR_FATAL;
if ( /* retry timestamp is not yet set */ ) {

/* set it */

}

if (now - timestamp > limit) return WL_SEND_ERROR_FATAL;

/* poll, using the timestamp to set a timeout */

return WL_SEND_ERROR_RETRY;

}

If your objection is that you would rather this handler be standardized, I
can move it to the the only patch, use the existing core client mutex, and
having the timestamp be part of the wl_display state. Multiple threads
would block for on the send, but they would have done so anyway. Then there
could be a public api that selected between the default (abort), error, or
error+retry implemented by the above pseudocode.

But as you said earlier, I will really instead have the wl_client_flush()
do the poll().

Post by Pekka Paalanen
How does the callback know at what kind of point of execution you are
in your application? Can you block there? Can you deal with
disconnection? What if the trigger fires inside a library you use, like
libEGL? Which holds mutexes of its own? What if it happens in a thread
you didn't expect, e.g. from GStreamer?
You'd probably need to maintain global flag variables across code
blocks. If you maintain something across code blocks, would it not be
easier instead to try a manual flush at the end of a block and see if
that works?

I agree that the retry option would only be useful in fairly restricted
cases, and not all clients could actually use it.

For the client I am dealing with, it is a valid option -- we know where all
the send calls are made, and are aware of the consequences of holding the
mutexes we do hold, and we are willing to block rather to immediately fail
the connection if it might recover.

But again, certainly cannot justify this part of the change by saying we
need it, because I'm fairly certain we do not (though perhaps I do need to
worry about fd's). For simplicity then I see no point really in arguing
further, and will strip out the retry option from my patch.

Post by Pekka Paalanen
If flushing is inefficient, we could look into adding ABI to get
information about the buffer status, so you can make a better choice on
whether to flush or not.
If you attempt a manual flush and it fails, you will have more freedom
to handle that situation than with the callback you propose. You can do
everything you could with the callback and you could also choose to
return from the request sending code block and come back later, e.g. on
the next round of your main event loop or when then Wayland fd signals
writable in poll() again.
Interrupting what you are doing is much easier if you can pick the
points where it is possible. It is hard to be able to handle a
potential callback every time you call a request sending function.

Especially if the client is a nested server, you probably would not

Post by Pekka Paalanen
want to block on the Wayland connection because that could prevent you
from servicing your own clients.

Interestingly while having that block does prevent the display from being
updated, it doesn't (immediately) block our clients, though they might
starve for buffers not being released fairly soon after. As a still
relatively uncommon error case, blocking briefly (and then
disconnecting/reconnecting if needed) is better than taking down everyone
due to the wl_abort.

Post by Pekka Paalanen
Do you think the alternative solutions could be workable for you?

As above, yes.

Post by Pekka Paalanen
I would be very happy to see someone working on those, this issue has
existed since the beginning.

I'm happy to help.

Post by Pekka Paalanen
Thanks,
pq

Post by Lloyd Pique
+ */
+typedef enum wl_send_error_strategy
+ (*wl_send_error_handler_func_t)(struct wl_display *, int /* errno

*/);

Post by Lloyd Pique
+
+void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t

handler);

Post by Lloyd Pique
+
void
wl_log_set_handler_client(wl_log_func_t handler);
diff --git a/src/wayland-client.c b/src/wayland-client.c
index efeb745..3a9db6f 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -113,6 +113,8 @@ struct wl_display {
int reader_count;
uint32_t read_serial;
pthread_cond_t reader_cond;
+
+ wl_send_error_handler_func_t send_error_handler;
};
/** \endcond */
@@ -696,6 +698,44 @@ wl_proxy_marshal_array_constructor(struct wl_proxy

*proxy,
proxy->version);

Post by Lloyd Pique
}
+static enum wl_send_error_strategy
+display_send_error_handler_invoke(struct wl_display *display, int

send_errno)

Post by Lloyd Pique
+{
+ enum wl_send_error_strategy ret = WL_SEND_ERROR_ABORT;
+ wl_send_error_handler_func_t handler = display->send_error_handler;
+ if (handler) {
+ pthread_mutex_unlock(&display->mutex);
+ ret = handler(display, send_errno);
+ pthread_mutex_lock(&display->mutex);
+ }
+ return ret;
+}
+
+static void
+display_closure_send_error(struct wl_display *display,
+ struct wl_closure *closure)
+{
+ while (true) {
+ int send_errno = errno;
+ switch (display_send_error_handler_invoke(display,

send_errno)) {

Post by Lloyd Pique
+ display_fatal_error(display, send_errno);
+ return;
+
+ if (!wl_closure_send(closure,

display->connection)) {

Post by Lloyd Pique
+ display_send_error_handler_invoke(display,

0);

Post by Lloyd Pique
+ return;
+ }
+ break;
+
+ wl_abort("Error sending request: %s\n",
+ strerror(send_errno));
+ }
+ }
+}
/** Prepare a request to be sent to the compositor
*
@@ -743,6 +783,9 @@ wl_proxy_marshal_array_constructor_versioned(struct

wl_proxy *proxy,

Post by Lloyd Pique
goto err_unlock;
}
+ if (proxy->display->last_error)
+ goto err_unlock;
+
closure = wl_closure_marshal(&proxy->object, opcode, args,

message);

Post by Lloyd Pique
if (closure == NULL)
wl_abort("Error marshalling request: %s\n",

strerror(errno));

Post by Lloyd Pique
@@ -751,7 +794,7 @@ wl_proxy_marshal_array_constructor_versioned(struct

wl_proxy *proxy,

Post by Lloyd Pique
wl_closure_print(closure, &proxy->object, true);
if (wl_closure_send(closure, proxy->display->connection))
- wl_abort("Error sending request: %s\n", strerror(errno));
+ display_closure_send_error(proxy->display, closure);
wl_closure_destroy(closure);
@@ -2000,6 +2043,28 @@ wl_display_flush(struct wl_display *display)
return ret;
}
+/** Sets the error handler to invoke when a send fails internally
+*
+* \param display The display context object
+* \param handler The function to call when an error occurs.
+*
+* Sets the error handler to call to decide what should happen on a send

error.

Post by Lloyd Pique
+*
+* If no handler is set, the client will use the WL_SEND_ERROR_ABORT

strategy.

Post by Lloyd Pique
+* This means the core client code will call abort() if if encounters a

failure

Post by Lloyd Pique
+* while sending a message to the server.
+*
+* Setting a handler allows a client implementation to alter this

behavior.

Post by Lloyd Pique
+*
+* \memberof wl_display
+*/
+WL_EXPORT void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t

handler)

Post by Lloyd Pique
+{
+ display->send_error_handler = handler;
+}
+
/** Set the user data associated with a proxy
*
* \param proxy The proxy object

Lloyd Pique

2018-06-19 02:54:23 UTC

Let me take things back a step. I was a bit too hasty in suggesting
something that would work for me for the fact that MAX_FDS_OUT is small. In
our client the buffer creation ends up being serialized, and so only one
thread will be creating buffers with the linux-dmabuf protocol at a time.

That might not be true for other clients, and so checking if the fd buffer
is almost full won't work. In fact I'm not so certain checking if it is
half full would work either... having more than 14 threads trying to create
such buffers at the same time does not seem unreasonable, and if more than
one fd were involved in each request (multi-plane formats), the number of
threads needed to hit the wl_abort anyway drops quickly. And there may be
some other protocol I'm not aware of that is worse.

Increasing the fd buffer would alleviate that. However it would introduce
another problem.

We also have wl_abort()'s from the call to wl_os_dupfd_cloexec() in
wl_closure_marshal() failing. Not very often, and we are willing to pass on
finding a fix for it for now, but increasing the number of fd's being held
for the transfer is definitely going to make that worse.

Perhaps making the writes be blocking is the only reasonable way after all?

What do you think?

Post by Lloyd Pique
Hi Pekka!
Thank you for the feedback. In the end, I think we have a good basic
agreement about the patch, and after reading your linked bug (
https://gitlab.freedesktop.org/wayland/wayland/issues/12), which I was
previously unaware of, it sounds like I will be just stripping out the
callback+retry case from my patch.
I will probably go ahead and add a function to check for the half-full
output buffer, and one you didn't think of to check whether MAX_FDS_OUT is
about to be exceeded by the next call (that count is small enough to
actually be a concern).
At this point I think the only real question I have is one of checking the
intent behind the bug you filed. Should the design be that ONLY behavior
is to set a fatal error on the display context, or should there be a public
API to select between the previous/default abort and the detal error?
I'll send out a revised patch after hearing back with that bit of guidance.
In case it matters, I did also include my responses to your points inline
below. I don't expect to convince that this patch should be used as is, but
perhaps it will give you or someone else some clarify and insight into why
I suggested the change I did.
Thanks!
- Lloyd

Post by Pekka Paalanen
On Tue, 5 Jun 2018 18:14:54 -0700

Post by Lloyd Pique
Introduce a new call wl_display_set_error_handler_client(), which allows
a client to register a callback function which is invoked if there is an
error (possibly transient) while sending messages to the server.
This allows a Wayland client that is actually a nested server to try and
recover more gracefully from send errors, allowing it one to disconnect
and reconnect to the server if necessary.
The handler is called with the wl_display and the errno, and is expected
to return one of WL_SEND_ERROR_ABORT, WL_SEND_ERROR_FATAL, or
WL_SEND_ERROR_RETRY. The core-client code will then respectively abort()
the process (the default if no handler is set), set the display context
into an error state, or retry the send operation that failed.7
The existing display test is extended to exercise the new function.

"Software"),

handling

Post by Lloyd Pique
+ *
+ * A client send error strategy handler is a callback function which

the client

Post by Lloyd Pique
+ * can set to direct the core client library how to respond if an

error is

Post by Lloyd Pique
+ * encountered while sending a message.
+ *
+ * Normally messages are enqueued to an output buffer by the core

client

Post by Lloyd Pique
+ * library, and the user of the core client library is expected to call
+ * wl_display_flush() occasionally, but as that is non-blocking,

messages can

Post by Lloyd Pique
+ * queue up in the output buffer. If later calls to send messages

happen to

Post by Lloyd Pique
+ * fill up the output buffer, the core client library will make an

internal

Post by Lloyd Pique
+ * call to flush the output buffer. But if the write call

unexpectedly fails,

Post by Lloyd Pique
+ * as there is no good way to return an error to the caller, the core

client

Post by Lloyd Pique
+ * library will log a message and call abort().

We actually do have a call to wl_display_flush() in our larger
implementation, called frequently as part of ensuring everything is
committed to the display. It does currently handle a return of -1, but by
waking up the input event processing thread in case the error is fatal, as
that thread handles disconnecting and reconnecting.
On the surface, making it detect an EAGAIN and wait kind of sounds
reasonable. But I kind of wonder how often in normal use it would return
EAGAIN, and so would force a wait that wouldn't need to, not when there is
still some space in the core client output buffer still. My reason for
adding the callback and the retry option were specifically to only make the
client block if there really was no other way to avoid an error state.
That said, I actually am willing to detect EAGAIN on wl_display_flush(),
and explicitly wait then. In trying out this patch, I only ended up
artificially reproducing an EAGAIN error (taking a few seconds to trigger)
when I magnified the request traffic by a factor of about 1000x. That is a
pretty unreasonable magnification even for a nested server, so while it
could happen, I don't think this is actually that important to my project.
In our case, the Wayland server we connect to is actually a small part of
a much larger Chrome UI process on ChromeOS. Despite being a user-space
process, it is not likely to be delayed in its processing of client
requests, and it seemed much more likely that the connection was dropping
for other reasons. Unfortunately I have no data on correlating our
anonymous crash reports (call stack only) with any data on why the Chrome
processes might have restarted (update? crash?). At least a clean reconnect
is much better than a wl_abort() for us, though still not a nice user
experience.

I believe I saw the exchange at one point in an IRC archive while looking
for existing known problems, though I though I remember seeing it for the
receive buffers.
Thinking about it fresh, Maybe that is reasonable as a general option for
clients. The one caveat I could think of has to do with MAX_FDS_OUT and it
being only 28. My project does use the linux-dmabuf-unstable-v1 protocol,
and having more than 14 surfaces in a nested server seems like it could
actually happen often. And on second thought, I could see my client filling
up all 28 in one frame, thanks to perhaps an ill-timed flood of
notifications in addition to normal UI surfaces.
Hmm, the patch I've uploaded does catch that, but if I remove the RETRY
option I'll think I will need to add another special case
wl_display_flush() check when creating buffers. In my case I would actually
prefer to find out that the fd buffer is completely full before
waiting/flushing though.

Post by Lloyd Pique
+ *
+ * If instead a send error strategy function is set, the core client

library

Post by Lloyd Pique
+ * will call it to allow the client to choose a different strategy.
+ *
+ * The function takes two arguments. The first is the display context

for the

Post by Lloyd Pique
+ * error. The second is the errno for the failure that occurred.
+ *
+ * The handler is expected to return one of the wl_send_error_strategy

values

Post by Lloyd Pique
+ * to indicate how the core client library should proceed.

This is the detail that bothers me: making the callback to determine
policy. You have no idea in what context that callback will be made.

Sure, but I meant this to be a choice available to the author of the
larger client, assuming they had full knowledge of what the consequences of
doing anything non-trivial were.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_ABORT, the core client code

will

Post by Lloyd Pique
+ * perform the default action -- log an error and then call the C

runtime

Post by Lloyd Pique
+ * abort() function.

This is the old behaviour, which we agree is not nice.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_FATAL, the core client code

will

Post by Lloyd Pique
+ * continue, but the display context will be put into a fatal error

state, and

Post by Lloyd Pique
+ * the display should no longer be used. Further calls to send messages
+ * will function, but the message will not actually be enqueued.

This is the behaviour I would prefer for unrecoverable errors. I
https://gitlab.freedesktop.org/wayland/wayland/issues/12

I hadn't seen that bug before now, and it exactly matches the conditions I
observed. Hence my above response that I will go ahead and strip down my
change to something simpler.

Post by Lloyd Pique
+ *
+ * If the handler returns WL_CLIENT_ERROR_RETRY, the send operation is

retried

Post by Lloyd Pique
+ * immediately on return, and may fail again. If it does, the handler

will be

Post by Lloyd Pique
+ * called again with the new errno. To avoid a busy loop, the handler

should

Post by Lloyd Pique
+ * wait on the display connection, or something equivalent. If after

returning

Post by Lloyd Pique
+ * WL_CLIENT_ERROR_RETRY the send operation succeeds, the handler is

called one

Post by Lloyd Pique
+ * last time with an errno of zero. This allows the handler to perform

any

Post by Lloyd Pique
+ * necessary cleanup now that the send operation has succeeded. For

this final

Post by Lloyd Pique
+ * call, the return value is ignored.
+ *
+ * Note that if the handler blocks, the current thread that is making

the call

Post by Lloyd Pique
+ * to send a message will of course be blocked. This means the default

behavior

Post by Lloyd Pique
+ * of Wayland client calls being non-blocking will no longer be true.
+ *
+ * In a multi threaded environment, this error handler could be called

once for

Post by Lloyd Pique
+ * every thread that attempts to send a message on the display and

encounters

Post by Lloyd Pique
+ * the error with the connection. It is the responsibility of the

handler

Post by Lloyd Pique
+ * function be itself thread safe.

This third case is the red flag for me.
How could someone use this efficiently in real life? Do you have
example users for this?

Unfortunately that code is not open source, otherwise I would point you at
it.
My actual use case is a self-contained process that happens to use Wayland
for input and output, and that the use of Wayland there is an internal
implementation detail. While my client is a nested server, it doesn't
expose a Wayland interface to its clients -- I had to support another
interface entirely.
To me there only seems to be one real general pattern for the callback
handler when might implement the retry strategy. It is just slightly more
complex than the retry_with_poll_handler() from the test I added.
int typical_retry_handler(struct wl_display *display, int send_errno) {
if (send_errno == 0) { /* retry succeeded */
/* clear retry start timestamp */
}
if (send_errno != EAGAIN) return WL_SEND_ERROR_FATAL;
if ( /* retry timestamp is not yet set */ ) {
/* set it */
}
if (now - timestamp > limit) return WL_SEND_ERROR_FATAL;
/* poll, using the timestamp to set a timeout */
return WL_SEND_ERROR_RETRY;
}
If your objection is that you would rather this handler be standardized, I
can move it to the the only patch, use the existing core client mutex, and
having the timestamp be part of the wl_display state. Multiple threads
would block for on the send, but they would have done so anyway. Then there
could be a public api that selected between the default (abort), error, or
error+retry implemented by the above pseudocode.
But as you said earlier, I will really instead have the wl_client_flush()
do the poll().

I agree that the retry option would only be useful in fairly restricted
cases, and not all clients could actually use it.
For the client I am dealing with, it is a valid option -- we know where
all the send calls are made, and are aware of the consequences of holding
the mutexes we do hold, and we are willing to block rather to immediately
fail the connection if it might recover.
But again, certainly cannot justify this part of the change by saying we
need it, because I'm fairly certain we do not (though perhaps I do need to
worry about fd's). For simplicity then I see no point really in arguing
further, and will strip out the retry option from my patch.

Especially if the client is a nested server, you probably would not

Post by Pekka Paalanen
want to block on the Wayland connection because that could prevent you
from servicing your own clients.

Post by Pekka Paalanen
Do you think the alternative solutions could be workable for you?

As above, yes.

Post by Pekka Paalanen
I would be very happy to see someone working on those, this issue has
existed since the beginning.

I'm happy to help.

Post by Pekka Paalanen
Thanks,
pq

Post by Lloyd Pique
+ */
+typedef enum wl_send_error_strategy
+ (*wl_send_error_handler_func_t)(struct wl_display *, int /* errno

*/);

Post by Lloyd Pique
+
+void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t

handler);

*proxy,
proxy->version);

Post by Lloyd Pique
}
+static enum wl_send_error_strategy
+display_send_error_handler_invoke(struct wl_display *display, int

send_errno)

Post by Lloyd Pique
+{
+ enum wl_send_error_strategy ret = WL_SEND_ERROR_ABORT;
+ wl_send_error_handler_func_t handler =

display->send_error_handler;

Post by Lloyd Pique
+ if (handler) {
+ pthread_mutex_unlock(&display->mutex);
+ ret = handler(display, send_errno);
+ pthread_mutex_lock(&display->mutex);
+ }
+ return ret;
+}
+
+static void
+display_closure_send_error(struct wl_display *display,
+ struct wl_closure *closure)
+{
+ while (true) {
+ int send_errno = errno;
+ switch (display_send_error_handler_invoke(display,

send_errno)) {

Post by Lloyd Pique
+ display_fatal_error(display, send_errno);
+ return;
+
+ if (!wl_closure_send(closure,

display->connection)) {

Post by Lloyd Pique
+

display_send_error_handler_invoke(display, 0);

wl_proxy *proxy,

Post by Lloyd Pique
goto err_unlock;
}
+ if (proxy->display->last_error)
+ goto err_unlock;
+
closure = wl_closure_marshal(&proxy->object, opcode, args,

message);

Post by Lloyd Pique
if (closure == NULL)
wl_abort("Error marshalling request: %s\n",

strerror(errno));

Post by Lloyd Pique
@@ -751,7 +794,7 @@ wl_proxy_marshal_array_constructor_versioned(struct

wl_proxy *proxy,

send error.

Post by Lloyd Pique
+*
+* If no handler is set, the client will use the WL_SEND_ERROR_ABORT

strategy.

Post by Lloyd Pique
+* This means the core client code will call abort() if if encounters a

failure

Post by Lloyd Pique
+* while sending a message to the server.
+*
+* Setting a handler allows a client implementation to alter this

behavior.

Post by Lloyd Pique
+*
+* \memberof wl_display
+*/
+WL_EXPORT void
+wl_display_set_send_error_handler_client(struct wl_display *display,
+ wl_send_error_handler_func_t

handler)

Post by Lloyd Pique
+{
+ display->send_error_handler = handler;
+}
+
/** Set the user data associated with a proxy
*
* \param proxy The proxy object

Pekka Paalanen

2018-06-19 12:43:43 UTC

On Mon, 18 Jun 2018 19:54:23 -0700

Post by Lloyd Pique
Let me take things back a step. I was a bit too hasty in suggesting
something that would work for me for the fact that MAX_FDS_OUT is small. In
our client the buffer creation ends up being serialized, and so only one
thread will be creating buffers with the linux-dmabuf protocol at a time.
That might not be true for other clients, and so checking if the fd buffer
is almost full won't work. In fact I'm not so certain checking if it is
half full would work either... having more than 14 threads trying to create
such buffers at the same time does not seem unreasonable, and if more than
one fd were involved in each request (multi-plane formats), the number of
threads needed to hit the wl_abort anyway drops quickly. And there may be
some other protocol I'm not aware of that is worse.
Increasing the fd buffer would alleviate that. However it would introduce
another problem.

Hi,

as I explained in my other email, the fd buffer can already hold 1k
fds. The problem in wl_connection_flush() seems to be that it runs out
of message data to flush out all the fds, so the fds can just keep
piling up. That seems like a problem worth solving on its own right.

Once that is solved, thresholding on the message data buffer
(soft-buffer) should be enough to keep the fd buffer never growing out
of bounds. Maybe then this would work:

- ABI to query a "flush recommended" flag; This flag would be set when
the soft-buffer is at least half-full, and cleared when it drops
to... below half? empty?

- When a client is doing lots of request sending without returning to
its main loop which would call wl_display_flush() anyway, it can
query the flag to see if it needs to flush.

- If flush ever fails, stop all request sending, poll for writable and
try again. How to do this is left for the application. Most
importantly, the application could set some state and return to its
main event loop to do other stuff in the mean while.

You're right that this wouldn't help an application that sends requests
from multiple threads a lot. They would need to be checking the flag
practically for every few requests, but at least that would be cheaper
than calling wl_display_flush() outright.

Post by Lloyd Pique
We also have wl_abort()'s from the call to wl_os_dupfd_cloexec() in
wl_closure_marshal() failing. Not very often, and we are willing to pass on
finding a fix for it for now, but increasing the number of fd's being held
for the transfer is definitely going to make that worse.

True. Can you think of any way to recover from dupfd failure without
disconnecting?

The very least it could be made to disconnect instead of abort.

Post by Lloyd Pique
Perhaps making the writes be blocking is the only reasonable way after all?
What do you think?

In my mind that would be a huge regression, so I wouldn't like it. If
we exhaust all other options, we could see about that.

Thanks,
pq

Lloyd Pique

2018-06-19 20:23:44 UTC

Hi Pekka,

- ABI to query a "flush recommended" flag; This flag would be set when

Post by Pekka Paalanen
the soft-buffer is at least half-full, and cleared when it drops
to... below half? empty?

This sounds reasonable, I'm happy to incorporate this first bit into my
patch, but .....

Post by Pekka Paalanen
- When a client is doing lots of request sending without returning to
its main loop which would call wl_display_flush() anyway, it can
query the flag to see if it needs to flush.
- If flush ever fails, stop all request sending, poll for writable and
try again. How to do this is left for the application. Most
importantly, the application could set some state and return to its
main event loop to do other stuff in the mean while.
You're right that this wouldn't help an application that sends requests
from multiple threads a lot. They would need to be checking the flag
practically for every few requests, but at least that would be cheaper
than calling wl_display_flush() outright.

[...]

Can you think of any way to recover from dupfd failure without

Post by Pekka Paalanen
disconnecting?

There are only two ways I can think of to recover from the dupfd failure:

1) Don't dup(). The purpose of it is to enqueue fds in the fds_out ring
buffer across multiple send calls. If instead a send call that had fd's did
an immediate flush, there would be no need for the dupe. The downside is
that the caller doesn't get a chance to do its wl_display_flush() under
conditions where it can do a wait.
2) Add a new "send-error" flag, breaking existing code, and requiring the
client to check after every send call and somehow do its own recovery and
retry the send calls.

I don't think anyone would want the second, except maybe as some
fundamental redesign that new clients were written anew for.

The first might actually be acceptable to me in practice.

Assuming the one wl_abort() after the call to wl_closure_send() becomes a
display_fatal_error(), and an EAGAIN here is converted to an EPIPE or other
fatal error, I don't think I will see EAGAIN very often if ever.

As you said, the kernel buffers are large. I don't know if that also
applies to fd's, but I would expect it has much larger limits there too.

But that is based on what I'm think I'm running into. Another client might
not want that.

I'm out of immediate ideas. What do you think?

- Lloyd

Lloyd Pique

2018-06-20 04:06:15 UTC

I did think of one more option to avoid the dup() abort.

- Open MAX_FDS_OUT to reserve a pool of fds (open /dev/nul or something
innocuous)
- Whenever the core client needs to dup a caller fd, use dup2/dup3 on an fd
in the pool
- Whenever the core client is done with the caller fd, use dup2/dup3 to
release it and point it back at something innocuous.

Separately, as my client is a service level nested server, I could
effectively configure it with setrlimit, and bump up the pool to the 1000
fds to fill the entire ring buffer. That would work, but may not be a
general solution for simpler clients, especially since the default limit is
only 1024.

If blocking is out, maybe the only thing I can do is add an option to
configure the amount of fd's the core will reserve/enqueue? However, while
it isn't a dangerous option, it also wouldn't just necessarily with a
default of 28, without the developer knowing what larger value to set.

(To make it clear, I'm thinking through this final issue with the fd limit
as best I can before I go ahead and revise my patch)

- Lloyd

Pekka Paalanen

2018-06-21 10:34:01 UTC

On Tue, 19 Jun 2018 21:06:15 -0700

Post by Lloyd Pique
I did think of one more option to avoid the dup() abort.
- Open MAX_FDS_OUT to reserve a pool of fds (open /dev/nul or something
innocuous)
- Whenever the core client needs to dup a caller fd, use dup2/dup3 on an fd
in the pool
- Whenever the core client is done with the caller fd, use dup2/dup3 to
release it and point it back at something innocuous.

Sorry, I don't think we are that desperate. :-)

I think we have established there probably isn't any reasonable way to
recover from dup() failure without disconnecting.

Post by Lloyd Pique
Separately, as my client is a service level nested server, I could
effectively configure it with setrlimit, and bump up the pool to the 1000
fds to fill the entire ring buffer. That would work, but may not be a
general solution for simpler clients, especially since the default limit is
only 1024.

Very rare client would even be sending tons of fds. Usually a client
has few buffers per wl_surface, and fds are sent when creating
wl_buffers but not when re-using them. A window being continuously
resized would be the usual cause of sending fds constantly since buffer
resize often implies allocation. Even then, the rate would be limited
to one buffer per display refresh and surface for a well-behaved client.

Outside buffers, the use of fds is very rare in general. This is why
you are the first to seriously hit and investigate this problem. You
have probably the first app that is hitting the 28 fds limit, at least
I don't recall hearing about such before.

Therefore I claim that most clients return to their main event loop to
sleep well before they have queued even a dozen fds. Your Wayland
client could have its rlimit raised, and sounds like it should have in
any case. This is why I think the dup() failure does not really need a
recovery mechanism that would keep the Wayland connection alive.

Thanks,
pq

Post by Lloyd Pique
If blocking is out, maybe the only thing I can do is add an option to
configure the amount of fd's the core will reserve/enqueue? However, while
it isn't a dangerous option, it also wouldn't just necessarily with a
default of 28, without the developer knowing what larger value to set.
(To make it clear, I'm thinking through this final issue with the fd limit
as best I can before I go ahead and revise my patch)
- Lloyd

Lloyd Pique

2018-08-06 21:54:16 UTC

Hi Pekka,

Sorry for the lack of any updates for (over) a month. I was on holiday for
a while, and had a few other things come up delaying me from getting back
to this.

I've created a first set of patches (send sets a fatal display error,
introduce "wl_display_soft_flush" which does a wl_display_flush only if
buffers are half-full), and I will try and get them uploaded soon.

I also have an almost completely untested code change which raises the
buffered fd limit (and constraints messages to at most three fd's). It
needs a test case, which I may or may not get to this week, as being
included and exercised in a more production environment.

As it's been a while, and I haven't looked at what is going on with the
move to gitlab for the Wayland project, should I still send my patches
here, or should I send them there?

Thanks!

- Lloyd Pique

Post by Pekka Paalanen
On Tue, 19 Jun 2018 21:06:15 -0700

Post by Lloyd Pique
in the pool
- Whenever the core client is done with the caller fd, use dup2/dup3 to
release it and point it back at something innocuous.

Sorry, I don't think we are that desperate. :-)
I think we have established there probably isn't any reasonable way to
recover from dup() failure without disconnecting.

Post by Lloyd Pique
only 1024.

Very rare client would even be sending tons of fds. Usually a client
has few buffers per wl_surface, and fds are sent when creating
wl_buffers but not when re-using them. A window being continuously
resized would be the usual cause of sending fds constantly since buffer
resize often implies allocation. Even then, the rate would be limited
to one buffer per display refresh and surface for a well-behaved client.
Outside buffers, the use of fds is very rare in general. This is why
you are the first to seriously hit and investigate this problem. You
have probably the first app that is hitting the 28 fds limit, at least
I don't recall hearing about such before.
Therefore I claim that most clients return to their main event loop to
sleep well before they have queued even a dozen fds. Your Wayland
client could have its rlimit raised, and sounds like it should have in
any case. This is why I think the dup() failure does not really need a
recovery mechanism that would keep the Wayland connection alive.
Thanks,
pq

Post by Lloyd Pique
If blocking is out, maybe the only thing I can do is add an option to
configure the amount of fd's the core will reserve/enqueue? However,

while

Post by Lloyd Pique
it isn't a dangerous option, it also wouldn't just necessarily with a
default of 28, without the developer knowing what larger value to set.
(To make it clear, I'm thinking through this final issue with the fd

limit

Post by Lloyd Pique
as best I can before I go ahead and revise my patch)
- Lloyd

Pekka Paalanen

2018-08-07 08:25:04 UTC

On Mon, 6 Aug 2018 14:54:16 -0700

Post by Lloyd Pique
Hi Pekka,
Sorry for the lack of any updates for (over) a month. I was on holiday for
a while, and had a few other things come up delaying me from getting back
to this.

Hi,

so was I! :-)

Post by Lloyd Pique
I've created a first set of patches (send sets a fatal display error,
introduce "wl_display_soft_flush" which does a wl_display_flush only if
buffers are half-full), and I will try and get them uploaded soon.
I also have an almost completely untested code change which raises the
buffered fd limit (and constraints messages to at most three fd's). It
needs a test case, which I may or may not get to this week, as being
included and exercised in a more production environment.

Sounds interesting, I only hope I will have time to look at it.

Post by Lloyd Pique
As it's been a while, and I haven't looked at what is going on with the
move to gitlab for the Wayland project, should I still send my patches
here, or should I send them there?

Wayland patches are still handled on the mailing list. Weston patches
will be preferred as Gitlab MRs very soon now, just need to get the
docs landed.

Thanks,
pq

Pekka Paalanen

2018-06-21 10:30:47 UTC

On Tue, 19 Jun 2018 13:23:44 -0700

Post by Lloyd Pique
Hi Pekka,
- ABI to query a "flush recommended" flag; This flag would be set when

Post by Pekka Paalanen
the soft-buffer is at least half-full, and cleared when it drops
to... below half? empty?

This sounds reasonable, I'm happy to incorporate this first bit into my
patch, but .....

[...]

Post by Pekka Paalanen
Can you think of any way to recover from dupfd failure without
disconnecting?

1) Don't dup(). The purpose of it is to enqueue fds in the fds_out ring
buffer across multiple send calls. If instead a send call that had fd's did
an immediate flush, there would be no need for the dupe. The downside is
that the caller doesn't get a chance to do its wl_display_flush() under
conditions where it can do a wait.

The purpose of dup() is to allow the program close() the fd
immediately after calling a message sending function. We also do not
want to flush on every request, that's the whole reason a buffer even
exists.

Or do you propose that dup() failure would trigger a flush and retry?

A flush could free more fd space, but yeah, implicit flushes always
have the issue with failing.

Post by Lloyd Pique
2) Add a new "send-error" flag, breaking existing code, and requiring the
client to check after every send call and somehow do its own recovery and
retry the send calls.
I don't think anyone would want the second, except maybe as some
fundamental redesign that new clients were written anew for.

Yes, there is a reason why message sending functions do not return a
status. It would be too hard to deal with aside from mostly ignoring it.

Post by Lloyd Pique
The first might actually be acceptable to me in practice.
Assuming the one wl_abort() after the call to wl_closure_send() becomes a
display_fatal_error(), and an EAGAIN here is converted to an EPIPE or other
fatal error, I don't think I will see EAGAIN very often if ever.
As you said, the kernel buffers are large. I don't know if that also
applies to fd's, but I would expect it has much larger limits there too.
But that is based on what I'm think I'm running into. Another client might
not want that.
I'm out of immediate ideas. What do you think?

Let's deal with one thing at a time. I'd suggest getting the full
fds_out capacity into use first, and see where that gets us. The second
step would be the "flush recommended" API.

If dup() becomes the next problem, we'll think about that then.
However, I'm still not convinced that dup() failure should be
recoverable without disconnecting. A program shouldn't be operating
near the fd space cap, like it shouldn't be operating near stack
exhaustion either. It comes down to what we would consider reasonable
in both behaviour and error recovery.

Thanks,
pq

Pekka Paalanen

2018-06-19 12:12:08 UTC

On Mon, 18 Jun 2018 18:41:28 -0700

Sounds good. I'll reply to your other email too.

Post by Lloyd Pique
At this point I think the only real question I have is one of checking the
intent behind the bug you filed. Should the design be that ONLY behavior
is to set a fatal error on the display context, or should there be a public
API to select between the previous/default abort and the detal error?

I think the aim should be to remove the abort(). I do not see any
reason to leave it in even as an option.

Post by Lloyd Pique
I'll send out a revised patch after hearing back with that bit of guidance.
In case it matters, I did also include my responses to your points inline
below. I don't expect to convince that this patch should be used as is, but
perhaps it will give you or someone else some clarify and insight into why
I suggested the change I did.

Nice, thank you for that. Some more below.

Post by Pekka Paalanen
On Tue, 5 Jun 2018 18:14:54 -0700

Post by Lloyd Pique
Introduce a new call wl_display_set_error_handler_client(), which allows
a client to register a callback function which is invoked if there is an
error (possibly transient) while sending messages to the server.
This allows a Wayland client that is actually a nested server to try and
recover more gracefully from send errors, allowing it one to disconnect
and reconnect to the server if necessary.
The handler is called with the wl_display and the errno, and is expected
to return one of WL_SEND_ERROR_ABORT, WL_SEND_ERROR_FATAL, or
WL_SEND_ERROR_RETRY. The core-client code will then respectively abort()
the process (the default if no handler is set), set the display context
into an error state, or retry the send operation that failed.7
The existing display test is extended to exercise the new function.

Hi Lloyd,
technically this looks like a quality submission, thank you for that. I
am particularly happy to see all the new cases are added into the test
suite. I agree that calling wl_abort() is nasty and it would be good to
have something else.
However, I'm not quite convinced of the introduced complexity here.
More comments inline.
If you know you are sending lots of requests, you should be calling
wl_display_flush() occasionally. It will return -1 if flushing fails to
let you know you need to wait. The main problem I see with that is when
to attempt to flush.

The soft-buffer in libwayland-client is quite small, the kernel socket
buffers are usually much larger in bytes. EAGAIN is triggered only when
the flush to the kernel fails, so I don't think that would happen
needlessly.

Post by Lloyd Pique
That said, I actually am willing to detect EAGAIN on wl_display_flush(),
and explicitly wait then. In trying out this patch, I only ended up
artificially reproducing an EAGAIN error (taking a few seconds to trigger)
when I magnified the request traffic by a factor of about 1000x. That is a
pretty unreasonable magnification even for a nested server, so while it
could happen, I don't think this is actually that important to my project.
Thinking about it fresh, Maybe that is reasonable as a general option for
clients. The one caveat I could think of has to do with MAX_FDS_OUT and it
being only 28. My project does use the linux-dmabuf-unstable-v1 protocol,
and having more than 14 surfaces in a nested server seems like it could
actually happen often. And on second thought, I could see my client filling
up all 28 in one frame, thanks to perhaps an ill-timed flood of
notifications in addition to normal UI surfaces.

If I'm reading the code right, MAX_FDS_OUT is only a limit for a single
sendmsg() call. The fds_out buffer is capable of holding 1k fds.

We have 4kB message data buffer (what I've been calling the
soft-buffer, as opposed to in-kernel buffers). A minimal request to
carry an fd would be header (8B) plus fd (4B), so that's 12B. That
makes at most 341 fds until the soft-buffer is full. That should well
fit into the 1k fds that fds_out can hold.

One could have more fds in a single request. Let's say four. The
request size becomes 24B carrying 4 fds. That makes 680 fds for a full
soft-buffer. Should still be fine.

However, wl_connection_flush() attempts to write out at most
MAX_FDS_OUT fds at a time, but all of the message data. OTOH, one
cannot even try to flush out fds without any message data, IIRC
sendmsg() doesn't support that. So it can run out of message data while
fds accumulate and finally fill fds_out up, causing irrecoverable
failure.

Do your observations support this theory?

It seems we should somehow flush less message data so that we can call
sendmsg() enough times to handle all the fds and avoid them piling up.
But if we do so, we must be very careful with any potential assumptions
on receiving side on the ordering between message data and related fds.

Post by Lloyd Pique
Hmm, the patch I've uploaded does catch that, but if I remove the RETRY
option I'll think I will need to add another special case
wl_display_flush() check when creating buffers. In my case I would actually
prefer to find out that the fd buffer is completely full before
waiting/flushing though.

Post by Lloyd Pique
+ *
+ * If instead a send error strategy function is set, the core client

library

Post by Lloyd Pique
+ * will call it to allow the client to choose a different strategy.
+ *
+ * The function takes two arguments. The first is the display context

for the

Post by Lloyd Pique
+ * error. The second is the errno for the failure that occurred.
+ *
+ * The handler is expected to return one of the wl_send_error_strategy

values

Post by Lloyd Pique
+ * to indicate how the core client library should proceed.

This is the detail that bothers me: making the callback to determine
policy. You have no idea in what context that callback will be made.

Sure, but I meant this to be a choice available to the author of the larger
client, assuming they had full knowledge of what the consequences of doing
anything non-trivial were.

Even the author of the client would usually not know. It would make a
very hard API to use correctly.

Post by Lloyd Pique
To me there only seems to be one real general pattern for the callback
handler when might implement the retry strategy. It is just slightly more
complex than the retry_with_poll_handler() from the test I added.
int typical_retry_handler(struct wl_display *display, int send_errno) {
if (send_errno == 0) { /* retry succeeded */
/* clear retry start timestamp */
}
if (send_errno != EAGAIN) return WL_SEND_ERROR_FATAL;
if ( /* retry timestamp is not yet set */ ) {
/* set it */
}
if (now - timestamp > limit) return WL_SEND_ERROR_FATAL;
/* poll, using the timestamp to set a timeout */
return WL_SEND_ERROR_RETRY;
}
If your objection is that you would rather this handler be standardized, I
can move it to the the only patch, use the existing core client mutex, and
having the timestamp be part of the wl_display state. Multiple threads
would block for on the send, but they would have done so anyway. Then there
could be a public api that selected between the default (abort), error, or
error+retry implemented by the above pseudocode.

My objection is to the automatic blocking in general. We have a pretty
nicely asynchronous protocol and we try to teach programmers to write
their apps in a non-blocking way, so blocking inside libwayland-client
is not nice. We have very few functions that are documented as
potentially blocking. Extending that to all request sending functions
is not acceptable in my mind.

If blocking is a suitable solution for a flush failure, I'd rather give
the user the choice of where and when to block. Then there is no need
to set even a timeout policy in libwayland-client.

Post by Lloyd Pique
But as you said earlier, I will really instead have the wl_client_flush()
do the poll().

Thanks,
pq

Lloyd Pique

2018-06-19 19:10:36 UTC

Hi Pekka,

Post by Pekka Paalanen
I think the aim should be to remove the abort(). I do not see any
reason to leave it in even as an option.

Thank you. Will proceeed in that direction.

Post by Pekka Paalanen
The soft-buffer in libwayland-client is quite small, the kernel socket
buffers are usually much larger in bytes. EAGAIN is triggered only when
the flush to the kernel fails, so I don't think that would happen
needlessly.

Ack.

Post by Pekka Paalanen
If I'm reading the code right, MAX_FDS_OUT is only a limit for a single
sendmsg() call. The fds_out buffer is capable of holding 1k fds.
We have 4kB message data buffer (what I've been calling the
soft-buffer, as opposed to in-kernel buffers). A minimal request to
carry an fd would be header (8B) plus fd (4B), so that's 12B. That
makes at most 341 fds until the soft-buffer is full. That should well
fit into the 1k fds that fds_out can hold.
One could have more fds in a single request. Let's say four. The
request size becomes 24B carrying 4 fds. That makes 680 fds for a full
soft-buffer. Should still be fine.

The limit is both the maximum amount per sendmsg() call, as well as the
maximum amount that can be sent by a single send request.

The fd's are added once at a time by a call to wl_connection_put_fd(),
which forces a flush once the limit is reached. Those calls are made by
copy_fds_to_connection(), called once per message send from
wl_closure_send().

On a flush (forced or not), the fd's are closed by close_fds(), which then
advances the tail to the head, emptying the ring buffer again.

That the output fd butter is over-large at 4k just allows much of the ring
buffer code to be reused. It will never actually hold 1024 fds enqueued for
output though.

(The input fd buffer may allow 1k fd's. I did not examine that carefully to
be sure).

I think this limit is still a problem. I could change it to actually allow
1k fd's to be queued up, but then that will make the dup() more likely to
fail (more in the other reply).

Post by Pekka Paalanen
However, wl_connection_flush() attempts to write out at most
MAX_FDS_OUT fds at a time, but all of the message data. OTOH, one
cannot even try to flush out fds without any message data, IIRC
sendmsg() doesn't support that. So it can run out of message data while
fds accumulate and finally fill fds_out up, causing irrecoverable
failure.
Do your observations support this theory?

I haven't observed a problem case like that. Any normal individual request
will probably only use a few fd's, and I don't think there is a way for a
request to not carry some non-fd data.

I can imagine someone creating a new protocol message that does take more
than 28 fd's in a request, where that would then fail to flush if there
were no other normal message data also already queued up. But that seems a
very abnormal use. wl_closure_marshal() would be the best place for a check
(it already checks for null objects -- see "goto err_null"), and I didn't
find other bound check on the number of fd's in a request on a quick look
through the code.

But I'll study the code a bit more in case there is some subtlety that
escapes me.

It seems we should somehow flush less message data so that we can call

Post by Pekka Paalanen
sendmsg() enough times to handle all the fds and avoid them piling up.
But if we do so, we must be very careful with any potential assumptions
on receiving side on the ordering between message data and related fds.

I expect that the receiving code will just pull the next fd out of the
fds_in ring buffer when turning the messages back into wl_closures for each
message, as it realizes an fd is needed for any argument.

But I haven't looked at the code. Another thing I will check to be safe.

Post by Pekka Paalanen
Even the author of the client would usually not know. It would make a
very hard API to use correctly.

Ok, I won't introduce anything like that.

Post by Pekka Paalanen
My objection is to the automatic blocking in general. We have a pretty
nicely asynchronous protocol and we try to teach programmers to write
their apps in a non-blocking way, so blocking inside libwayland-client
is not nice. We have very few functions that are documented as
potentially blocking. Extending that to all request sending functions
is not acceptable in my mind.

I agree and appreciate that the core library is non-blocking, and really
don't want to change that, which is why my original patch made that
blocking at least opt-in, and even then NOT even part of the core client
code.

- Lloyd

Pekka Paalanen

2018-06-21 10:29:09 UTC

On Tue, 19 Jun 2018 12:10:36 -0700

Post by Lloyd Pique
Hi Pekka,

Hi,

ah, so there is an artificial limit there as well. I missed that. So
that is what prevents the fds from accumulating in fds_out indefinitely.

Post by Lloyd Pique
On a flush (forced or not), the fd's are closed by close_fds(), which then
advances the tail to the head, emptying the ring buffer again.
That the output fd butter is over-large at 4k just allows much of the ring
buffer code to be reused. It will never actually hold 1024 fds enqueued for
output though.
(The input fd buffer may allow 1k fd's. I did not examine that carefully to
be sure).
I think this limit is still a problem. I could change it to actually allow
1k fd's to be queued up, but then that will make the dup() more likely to
fail (more in the other reply).

Yes, upping it to 1k for real would be my suggestion. I think it should
be large enough to accommodate any amount of fds so that the limiting
factor will be the soft-buffer size.

I think the 4 fds in a minimal message is a reasonable upper limit. It
is very rare to find even two fds in a single message, because such a
message cannot be sent at all without all the declared fds. There is no
way to send a "null" fd, so protocol design should favour one message
per fd when the number of fds is variable.

I haven't observed a problem case like that. Any normal individual request
will probably only use a few fd's, and I don't think there is a way for a
request to not carry some non-fd data.
I can imagine someone creating a new protocol message that does take more
than 28 fd's in a request, where that would then fail to flush if there
were no other normal message data also already queued up. But that seems a
very abnormal use. wl_closure_marshal() would be the best place for a check
(it already checks for null objects -- see "goto err_null"), and I didn't
find other bound check on the number of fd's in a request on a quick look
through the code.
But I'll study the code a bit more in case there is some subtlety that
escapes me.

Yeah, there probably isn't any upper limit coded for number of fds in a
single message, or number of arguments, aside from a generic send
failure.

I believe it would be good to introduce an upper limit of maybe 4 fds
per message. Or at least suggest it. It should be done in both
wayland-scanner and libwayland.

Post by Pekka Paalanen
It seems we should somehow flush less message data so that we can call
sendmsg() enough times to handle all the fds and avoid them piling up.
But if we do so, we must be very careful with any potential assumptions
on receiving side on the ordering between message data and related fds.

I expect that the receiving code will just pull the next fd out of the
fds_in ring buffer when turning the messages back into wl_closures for each
message, as it realizes an fd is needed for any argument.
But I haven't looked at the code. Another thing I will check to be safe.

That is my understanding as well, which means that fds must arrive with
or before the corresponding other message data. They cannot arrive
later. It should be ok to lift this restriction if necessary, but maybe
it's better to make sure data cannot arrive before fds.

Thanks,
pq