<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 26 November 2014 at 13:48, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 21 Nov 2014 11:12:35 +0100 Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > Remove out-dated documentation and add few more words > about this topic. > > Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> > --- > src/wayland-client.c | 106 ++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 87 insertions(+), 19 deletions(-) > > diff --git a/src/wayland-client.c b/src/wayland-client.c > index 41c49e9..b1a1fa0 100644 > --- a/src/wayland-client.c > +++ b/src/wayland-client.c > @@ -909,6 +909,12 @@ static const struct wl_callback_listener sync_listener = { > * Blocks until the server process all currently issued requests and > * sends out pending events on the event queue. > * > + * \note This function uses wl_display_dispatch_queue() internally. If you > + * are using wl_display_read_events() from more threads, don't use this function > + * (or make sure that calling wl_display_roundtrip_queue() doesn't interfere > + * with calling wl_display_prepare_read() and wl_display_read_events()) > + * > + * \sa wl_display_roundtrip() > * \memberof wl_display > */ > WL_EXPORT int > @@ -940,6 +946,11 @@ wl_display_roundtrip_queue(struct wl_display *display, struct wl_event_queue *qu > * Blocks until the server process all currently issued requests and > * sends out pending events on the default event queue. > * > + * \note This function uses wl_display_dispatch_queue() internally. If you > + * are using wl_display_read_events() from more threads, don't use this function > + * (or make sure that calling wl_display_roundtrip() doesn't interfere > + * with calling wl_display_prepare_read() and wl_display_read_events()) > + * > * \memberof wl_display > */ > WL_EXPORT int > @@ -1211,14 +1222,55 @@ cancel_read(struct wl_display *display) > * > * This will read events from the file descriptor for the display. > * This function does not dispatch events, it only reads and queues > - * events into their corresponding event queues. If no data is > + * events into their corresponding event queues. If no data is > * avilable on the file descriptor, wl_display_read_events() returns > - * immediately. To dispatch events that may have been queued, call > - * wl_display_dispatch_pending() or > - * wl_display_dispatch_queue_pending(). > + * immediately. To dispatch events that may have been queued, call > + * wl_display_dispatch_pending() or wl_display_dispatch_queue_pending(). > * > * Before calling this function, wl_display_prepare_read() must be > - * called first. > + * called first. When running in more threads (which is the usual > + * case, since we'd use wl_display_dispatch() otherwise), every thread > + * must call wl_display_prepare_read() before calling this function. > + * > + * After calling wl_display_prepare_read() there can be some extra code > + * before calling wl_display_read_events(), for example poll() or alike. > + * Example of code in a thread: > + * > + * \code > + * > + * while (wl_display_prepare_read(display) < 0) > + * wl_display_dispatch_pending(display); > + * wl_display_flush(display); > + * > + * ... some code ... > + * > + * fds[0].fd = wl_display_get_fd(display); > + * fds[0].event = POLLIN | POLLHUP | POLLERR; > + * poll(fds, 1, -1); > + * > + * if (!everything_ok()) { > + * wl_display_cancel_read(display); > + * handle_error(); > + * } > + * > + * if (wl_display_read_events(display) < 0) > + * handle_error(); > + * > + * ... > + * \endcode > + * > + * The code should be as short as possible since other threads may > + * get sleeping until all threads called wl_display_read_events() > + * or wl_display_cancel_read(). </div></div>Hi, this paragraph is a bit confusing, "short code" is not a clear term. There is also the poll() which may take an arbitary amount of time to return. I probably see what you are getting at, but it should be rephrased. How about something like: After wl_display_prepare_read() succeeds, other threads that enter wl_display_read_events() will sleep until the very last thread enters it too or cancels. Therefore when the display fd becomes (or already is) readable, wl_display_read_events() should be called as soon as possible to unblock all threads. If wl_display_read_events() will not be called, then wl_display_cancel_read() must be called posthaste instead to let the other threads continue. </blockquote><div> </div><div>Yes, that's clearer :) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + * > + * This function must not be called simultaneously with wl_display_dispatch(). > + * It may lead to deadlock. If programmer wants, for some reason, use > + * wl_display_dispatch() in one thread and wl_display_read_events() in another, > + * extra care must be taken to serialize these calls, i. e. use mutexes or > + * similar. Hmm, that mutex should protect not only wl_display_read_events() call, but the whole sequence between prepare_read and read_events/cancel_read, should it not? </blockquote><div> </div><div>Yup </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + * > + * \sa wl_display_prepare_read(), wl_display_cancel_read(), > + * wl_display_dispatch_pending(), wl_display_dispatch() > * > * \memberof wl_display > */ > @@ -1296,17 +1348,17 @@ wl_display_prepare_read_queue(struct wl_display *display, > return ret; > } > > -/** Prepare to read events after polling file descriptor > +/** Prepare to read events from the display's file descriptor > * > * \param display The display context object > * \return 0 on success or -1 if event queue was not empty > * > * This function must be called before reading from the file > - * descriptor using wl_display_read_events(). Calling > - * wl_display_prepare_read() announces the calling threads intention > + * descriptor using wl_display_read_events(). Calling > + * wl_display_prepare_read() announces the calling thread's intention > * to read and ensures that until the thread is ready to read and > * calls wl_display_read_events(), no other thread will read from the > - * file descriptor. This only succeeds if the event queue is empty > + * file descriptor. This only succeeds if the event queue is empty > * though, and if there are undispatched events in the queue, -1 is > * returned and errno set to EAGAIN. > * > @@ -1314,6 +1366,13 @@ wl_display_prepare_read_queue(struct wl_display *display, > * either call wl_display_read_events() when it's ready or cancel the > * read intention by calling wl_display_cancel_read(). > * > + * This function doesn't acquire exclusive access to the display's fd. > + * It only registers that the thread calling this function has intention to > + * read from fd. When all registered readers call wl_display_read_events(), > + * only one (at random) eventually reads and queues the events and the > + * others are sleeping meanwhile. This way we avoid races and still > + * can read from more threads. > + * > * Use this function before polling on the display fd or to integrate > * the fd into a toolkit event loop in a race-free way. Typically, a > * toolkit will call wl_display_dispatch_pending() before sleeping, to > @@ -1350,9 +1409,10 @@ wl_display_prepare_read_queue(struct wl_display *display, > * Here we call wl_display_prepare_read(), which ensures that between > * returning from that call and eventually calling > * wl_display_read_events(), no other thread will read from the fd and > - * queue events in our queue. If the call to > - * wl_display_prepare_read() fails, we dispatch the pending events and > - * try again until we're successful. > + * queue events in our queue. If the call to wl_display_prepare_read() fails, > + * we dispatch the pending events and try again until we're successful. > + * > + * \sa wl_display_cancel_read(), wl_display_read_events() > * > * \memberof wl_display > */ > @@ -1362,13 +1422,15 @@ wl_display_prepare_read(struct wl_display *display) > return wl_display_prepare_read_queue(display, &display->default_queue); > } > > -/** Release exclusive access to display file descriptor > +/** Cancel read intention on display's fd > * > * \param display The display context object > * > - * This releases the exclusive access. Useful for canceling the lock > - * when a timed out poll returns fd not readable and we're not going > - * to read from the fd anytime soon. > + * After a thread successfully called wl_display_preapare_read() it must </div></div>Typo: preapare. > + * either call wl_display_read_events() or wl_display_cancel_read(). > + * If the threads do not follow this rule it will lead to deadlock. > + * > + * \sa wl_display_prepare_read(), wl_display_read_events() > * > * \memberof wl_display > */ > @@ -1396,6 +1458,9 @@ wl_display_cancel_read(struct wl_display *display) > * threads this will block until the main thread queues events on the queue > * passed as argument. > * > + * For behaviour in multi-threaded environment see wl_display_read_events() > + * and wl_display_prepare_read() Shouldn't we have the same warning here for wl_display_dispatch_queue() as we have below for wl_display_dispatch()? Only the *_pending() function are safe in a multithreaded app, right? </blockquote><div> </div><div>Yes, only the ones that do not attempt to read from the fd. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + * > * \memberof wl_display > */ > WL_EXPORT int > @@ -1502,10 +1567,13 @@ wl_display_dispatch_queue_pending(struct wl_display *display, > * or not. For dispatching main queue events without blocking, see \ref > * wl_display_dispatch_pending(). > * > - * \note Calling this will release the display file descriptor if this > - * thread acquired it using wl_display_acquire_fd(). > + * In multi-threaded environment, programmer may want to use > + * wl_display_read_events(). However, use of wl_display_read_events() > + * must not be mixed with wl_display_dispatch(). See wl_display_read_events() > + * and wl_display_prepare_read() for more details. > * > - * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue() > + * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue(), > + * wl_display_read_events() > * > * \memberof wl_display > */ Meanwhile I have merged some patches that make this one need a rebase. Otherwise it looks ok to me. Thanks, pq </blockquote><div> </div></div> </div><div class="gmail_extra">Thanks, </div><div class="gmail_extra">Marek </div><div class="gmail_extra"> </div></div>