[v3,2/2] client: update documentation about threading

Submitted by Marek Chalupa on Dec. 3, 2014, 2:53 p.m.

Details

Message ID 1417618397-15785-2-git-send-email-mchqwerty@gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Marek Chalupa Dec. 3, 2014, 2:53 p.m.
Remove out-dated documentation and add few more words
about this topic.

v2. replace a paragraph by better explanation from Pekka Paalanen
    fix other notes from reviewing

v3. fix typo

Signed-off-by: Marek Chalupa <mchqwerty@gmail.com>
---
 src/wayland-client.c | 104 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 90 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/wayland-client.c b/src/wayland-client.c
index eae438a..84b15a6 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
@@ -1216,14 +1227,59 @@  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
  * available 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
+ *
+ * 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 instead to let
+ * the other threads continue.
+ *
+ * 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_prepare_read() with
+ * wl_display_read_events() in another, extra care must be taken to serialize
+ * these calls, i. e. use mutexes or similar (on whole prepare + read sequence)
+ *
+ * \sa wl_display_prepare_read(), wl_display_cancel_read(),
+ * wl_display_dispatch_pending(), wl_display_dispatch()
  *
  * \memberof wl_display
  */
@@ -1301,17 +1357,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.
  *
@@ -1319,6 +1375,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
@@ -1366,6 +1429,8 @@  wl_display_prepare_read_queue(struct wl_display *display,
  * wl_display_prepare_read_queue() and wl_display_dispatch_queue_pending()
  * need to be used instead.
  *
+ * \sa wl_display_cancel_read(), wl_display_read_events()
+ *
  * \memberof wl_display
  */
 WL_EXPORT int
@@ -1374,13 +1439,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_prepare_read() it must
+ * 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
  */
@@ -1420,6 +1487,9 @@  wl_display_cancel_read(struct wl_display *display)
  * That means that this function can return non-0 value even when it
  * haven't dispatched any event for the given queue.
  *
+ * This function has the same constrains for using in multi-threaded apps
+ * as \ref wl_display_dispatch().
+ *
  * \sa wl_display_dispatch(), wl_display_dispatch_pending(),
  * wl_display_dispatch_queue_pending()
  *
@@ -1525,11 +1595,17 @@  wl_display_dispatch_queue_pending(struct wl_display *display,
  * the appropriate event queues. Finally, events on the default event queue
  * are dispatched.
  *
+ * 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.
+ *
  * \note It is not possible to check if there are events on the queue
  * or not. For dispatching default queue events without blocking, see \ref
  * wl_display_dispatch_pending().
  *
- * \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
  */

Comments

Hi,

On 3 December 2014 at 14:53, Marek Chalupa <mchqwerty@gmail.com> wrote:

> + *   fds[0].fd = wl_display_get_fd(display);
> + *   fds[0].event = POLLIN | POLLHUP | POLLERR;
>

POLLHUP and POLLERR are not valid for fds[0].events (note spelling); they
can be returned in revents if these events happened, but you don't
explicitly select for them.

Other than that, these look good to me, with the caveat that it might be
nice to invert the order of prepare_read()'s documentation: currently it
talks about the problem the function solves, and only later goes on to
explain the pattern you should actually use. It would be nice to just
explain what to do, and only later explain why you shouldn't do anything
else.

Thanks for doing this! It's super-helpful, and magically resolves one of
the things on my post-it TODO note. ;)

Cheers,
Daniel
On 3 December 2014 at 17:04, Daniel Stone <daniel@fooishbar.org> wrote:

> Hi,
>
> On 3 December 2014 at 14:53, Marek Chalupa <mchqwerty@gmail.com> wrote:
>
>> + *   fds[0].fd = wl_display_get_fd(display);
>> + *   fds[0].event = POLLIN | POLLHUP | POLLERR;
>>
>
> POLLHUP and POLLERR are not valid for fds[0].events (note spelling); they
> can be returned in revents if these events happened, but you don't
> explicitly select for them.
>

Well, to be precise -- they are valid but have no effect (according to man
pages).
Fixed, thanks :)


>
> Other than that, these look good to me, with the caveat that it might be
> nice to invert the order of prepare_read()'s documentation: currently it
> talks about the problem the function solves, and only later goes on to
> explain the pattern you should actually use. It would be nice to just
> explain what to do, and only later explain why you shouldn't do anything
> else.
>

Yeah, that sounds reasonable. I swapped these parts and also changed the
example code a little bit.



> Thanks for doing this! It's super-helpful, and magically resolves one of
> the things on my post-it TODO note. ;)
>
> Cheers,
> Daniel
>

Thanks,
Marek
On Fri, 5 Dec 2014 14:03:28 +0100
Marek Chalupa <mchqwerty@gmail.com> wrote:

> On 3 December 2014 at 17:04, Daniel Stone <daniel@fooishbar.org> wrote:
> 
> > Hi,
> >
> > On 3 December 2014 at 14:53, Marek Chalupa <mchqwerty@gmail.com> wrote:
> >
> >> + *   fds[0].fd = wl_display_get_fd(display);
> >> + *   fds[0].event = POLLIN | POLLHUP | POLLERR;
> >>
> >
> > POLLHUP and POLLERR are not valid for fds[0].events (note spelling); they
> > can be returned in revents if these events happened, but you don't
> > explicitly select for them.
> >
> 
> Well, to be precise -- they are valid but have no effect (according to man
> pages).
> Fixed, thanks :)
> 
> 
> >
> > Other than that, these look good to me, with the caveat that it might be
> > nice to invert the order of prepare_read()'s documentation: currently it
> > talks about the problem the function solves, and only later goes on to
> > explain the pattern you should actually use. It would be nice to just
> > explain what to do, and only later explain why you shouldn't do anything
> > else.
> >
> 
> Yeah, that sounds reasonable. I swapped these parts and also changed the
> example code a little bit.
> 
> 
> 
> > Thanks for doing this! It's super-helpful, and magically resolves one of
> > the things on my post-it TODO note. ;)

Both patches pushed. I took the liberty to add R-b Daniel, too. Awesome.


Thanks,
pq