Fix a race between track startup and scratching. Basically if the

scratch was too soon then SM_CANCEL would arrive at the speaker before
SM_PLAY, leaving the speaker thinking this was a queue removal rather
than a scratch, and therefore not sending a response.

The fix is to respond to _all_ SM_CANCELs whatever the speaker thinks
they are, and disorderd to always check the ID against the playing
track.  The responses are distinguished, but the server no longer uses
this information.

diff --git a/lib/speaker-protocol.h b/lib/speaker-protocol.h
index 520a0c6..92ce747 100644 (file)
--- a/lib/speaker-protocol.h
+++ b/lib/speaker-protocol.h
@@ -99,6 +99,9 @@ struct speaker_message {
  * initialization. */
 #define SM_READY 132
 
+/** @brief Cancelled track @c id which wasn't playing */
+#define SM_STILLBORN 133
+
 void speaker_send(int fd, const struct speaker_message *sm);
 /* Send a message. */
 

diff --git a/server/play.c b/server/play.c
index d73aec7..c447aa7 100644 (file)
--- a/server/play.c
+++ b/server/play.c
@@ -111,13 +111,9 @@ static int speaker_readable(ev_source *ev, int fd,
     D(("SM_PAUSED %s %ld", sm.id, sm.data));
     playing->sofar = sm.data;
     break;
-  case SM_FINISHED:
-    /* the playing track finished */
-    D(("SM_FINISHED %s", sm.id));
-    finished(ev);
-    break;
-  case SM_UNKNOWN:
-    /* we asked for an unknown track to be cancelled */
+  case SM_FINISHED:                    /* scratched the playing track */
+  case SM_STILLBORN:                   /* scratched too early */
+  case SM_UNKNOWN:                     /* scratched WAY too early */
     if(playing && !strcmp(sm.id, playing->id))
       finished(ev);
     break;

diff --git a/server/speaker.c b/server/speaker.c
index 407b1d7..7411a8e 100644 (file)
--- a/server/speaker.c
+++ b/server/speaker.c
@@ -553,21 +553,31 @@ static void mainloop(void) {
           report();
          break;
        case SM_CANCEL:
-          D(("SM_CANCEL %s",  sm.id));
+          D(("SM_CANCEL %s", sm.id));
          t = removetrack(sm.id);
          if(t) {
            if(t == playing) {
+              /* scratching the playing track */
               sm.type = SM_FINISHED;
-              strcpy(sm.id, playing->id);
-              speaker_send(1, &sm);
              playing = 0;
+            } else {
+              /* Could be scratching the playing track before it's quite got
+               * going, or could be just removing a track from the queue.  We
+               * log more because there's been a bug here recently than because
+               * it's particularly interesting; the log message will be removed
+               * if no further problems show up. */
+              info("SM_CANCEL for nonplaying track %s", sm.id);
+              sm.type = SM_STILLBORN;
             }
+            strcpy(sm.id, t->id);
            destroy(t);
          } else {
+            /* Probably scratching the playing track well before it's got
+             * going, but could indicate a bug, so we log this as an error. */
             sm.type = SM_UNKNOWN;
-            speaker_send(1, &sm);
            error(0, "SM_CANCEL for unknown track %s", sm.id);
           }
+          speaker_send(1, &sm);
           report();
          break;
        case SM_RELOAD: