agent: Document correctness of io_mutex locking

3 files changed, 24 insertions, 3 deletions

diff --git a/agent/agent.c b/agent/agent.c
index b9bcf3b..98fe893 100644
--- a/agent/agent.c
+++ b/agent/agent.c
@@ -1019,6 +1019,7 @@ pseudo_tcp_socket_opened (PseudoTcpSocket *sock, gpointer user_data)
       stream->id, component->id);
 }
 
+/* This is called with the agent lock held. */
 static void
 pseudo_tcp_socket_readable (PseudoTcpSocket *sock, gpointer user_data)
 {
@@ -1039,7 +1040,10 @@ pseudo_tcp_socket_readable (PseudoTcpSocket *sock, gpointer user_data)
   has_io_callback = component_has_io_callback (component);
 
   do {
-    /* Only dequeue pseudo-TCP data if we can reliably inform the client. */
+    /* Only dequeue pseudo-TCP data if we can reliably inform the client. The
+     * agent lock is held here, so has_io_callback can only change during
+     * component_emit_io_callback(), after which it’s re-queried. This ensures
+     * no data loss of packets already received and dequeued. */
     if (has_io_callback) {
       len = pseudo_tcp_socket_recv (sock, (gchar *) buf, sizeof(buf));
     } else if (component->recv_buf != NULL) {
@@ -3067,7 +3071,11 @@ component_io_cb (GSocket *socket, GIOCondition condition, gpointer user_data)
 
   /* Choose which receive buffer to use. If we’re reading for
    * nice_agent_attach_recv(), use a local static buffer. If we’re reading for
-   * nice_agent_recv(), use the buffer provided by the client. */
+   * nice_agent_recv(), use the buffer provided by the client.
+   *
+   * has_io_callback cannot change throughout this function, as we operate
+   * entirely with the agent lock held, and component_set_io_callback() would
+   * need to take the agent lock to change the Component’s io_callback. */
   g_assert (!has_io_callback || component->recv_buf == NULL);
 
   if (has_io_callback) {
diff --git a/agent/agent.h b/agent/agent.h
index 649e715..5718479 100644
--- a/agent/agent.h
+++ b/agent/agent.h
@@ -690,6 +690,12 @@ nice_agent_restart (
  * This must not be used in combination with nice_agent_recv() (or
  * #NiceIOStream or #NiceInputStream) on the same stream/component pair.
  *
+ * Calling nice_agent_attach_recv() with a %NULL @func will detach any existing
+ * callback and cause reception to be paused for the given stream/component
+ * pair. You must iterate the previously specified #GMainContext sufficiently to
+ * ensure all pending I/O callbacks have been received before calling this
+ * function to unset @func, otherwise data loss of received packets may occur.
+ *
  * Returns: %TRUE on success, %FALSE if the stream or component IDs are invalid.
  */
 gboolean
diff --git a/agent/component.c b/agent/component.c
index 7e5f79b..590ebcf 100644
--- a/agent/component.c
+++ b/agent/component.c
@@ -555,7 +555,14 @@ component_set_io_context (Component *component, GMainContext *context)
 
 /* (func, user_data) and (recv_buf, recv_buf_len) are mutually exclusive.
  * At most one of the two must be specified; if both are NULL, the Component
- * will not receive any data (i.e. reception is paused). */
+ * will not receive any data (i.e. reception is paused).
+ *
+ * Apart from during setup, this must always be called with the agent lock held,
+ * and the I/O lock released (because it takes the I/O lock itself). Requiring
+ * the agent lock to be held means it can’t be called between a packet being
+ * dequeued from the kernel buffers in agent.c, and an I/O callback being
+ * emitted for it (which could cause data loss if the I/O callback function was
+ * unset in that time). */
 void
 component_set_io_callback (Component *component,
     NiceAgentRecvFunc func, gpointer user_data,