Re: Help with solving a thread safety issue

[Jeremy White <jwhite@xxxxxxxxxxxxxxx>]

>    2.  Surgically modify the snd_worker.c calls that handle the Playback
> channel to introduce a mutex; lock and release that mutex around
> operations.  This is the direction I'm leaning, but I don't see a lot of
> evidence that this is commonly done.

Now, with patch that solves my problem.  Stop me now or I'll submit it...

Cheers,

Jeremy

diff --git a/server/snd_worker.c b/server/snd_worker.c
index 70148b7..b977bed 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -115,6 +115,8 @@ struct SndChannel {
     snd_channel_handle_message_proc handle_message;
     snd_channel_on_message_done_proc on_message_done;
     snd_channel_cleanup_channel_proc cleanup;
+
+    pthread_mutex_t lock;
 };
 
 typedef struct PlaybackChannel PlaybackChannel;
@@ -220,8 +222,21 @@ static void snd_disconnect_channel(SndChannel *channel)
     spice_marshaller_destroy(channel->send_data.marshaller);
     snd_channel_put(channel);
     worker->connection = NULL;
+
+    pthread_mutex_destroy(&channel->lock);
+}
+
+static int snd_lock(SndChannel *channel)
+{
+    return pthread_mutex_lock(&channel->lock);
+}
+
+static void snd_unlock(SndChannel *channel)
+{
+    pthread_mutex_unlock(&channel->lock);
 }
 
+
 static void snd_playback_free_frame(PlaybackChannel *playback_channel, AudioFrame *frame)
 {
     frame->channel = playback_channel;
@@ -967,6 +982,8 @@ static SndChannel *__new_channel(SndWorker *worker, int size, uint32_t channel_i
     if (!channel->channel_client) {
         goto error2;
     }
+
+    pthread_mutex_init(&channel->lock, NULL);
     return channel;
 
 error2:
@@ -1105,10 +1122,15 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance
     frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
     playback_channel = frame->channel;
     spice_assert(playback_channel);
+
+    if (snd_lock(&playback_channel->base) != 0)
+        return;
+
     if (!snd_channel_put(&playback_channel->base) ||
         sin->st->worker.connection != &playback_channel->base) {
         /* lost last reference, channel has been destroyed previously */
         spice_info("audio samples belong to a disconnected channel");
+        snd_unlock(&playback_channel->base);
         return;
     }
     spice_assert(playback_channel->base.active);
@@ -1121,6 +1143,7 @@ SPICE_GNUC_VISIBLE void spice_server_playback_put_samples(SpicePlaybackInstance
     playback_channel->pending_frame = frame;
     snd_set_command(&playback_channel->base, SND_PLAYBACK_PCM_MASK);
     snd_playback_send(&playback_channel->base);
+    snd_unlock(&playback_channel->base);
 }
 
 void snd_set_playback_latency(RedClient *client, uint32_t latency)
@@ -1136,8 +1159,11 @@ void snd_set_playback_latency(RedClient *client, uint32_t latency)
                 PlaybackChannel* playback = (PlaybackChannel*)now->connection;
 
                 playback->latency = latency;
-                snd_set_command(now->connection, SND_PLAYBACK_LATENCY_MASK);
-                snd_playback_send(now->connection);
+                if (snd_lock(now->connection) == 0) {
+                    snd_set_command(now->connection, SND_PLAYBACK_LATENCY_MASK);
+                    snd_playback_send(now->connection);
+                    snd_unlock(now->connection);
+                }
             } else {
                 spice_debug("client doesn't not support SPICE_PLAYBACK_CAP_LATENCY");
             }

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel