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 520a0c6d4ba3369fad952acaa003a2ae7fbbda8a..92ce747f10f882136ad08759d2ce8d550e119641 100644 (file)
--- a/lib/speaker-protocol.h
+++ b/lib/speaker-protocol.h
@@ -99,6 +99,9 @@ struct speaker_message {
  * initialization. */
 #define SM_READY 132
 
  * 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. */
 
 void speaker_send(int fd, const struct speaker_message *sm);
 /* Send a message. */
 

diff --git a/server/play.c b/server/play.c
index d73aec7482bac24cedd54268e96479d186dc14f8..c447aa798d18487970b6fcd9edb53ce2f7aab5b8 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;
     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;
     if(playing && !strcmp(sm.id, playing->id))
       finished(ev);
     break;

diff --git a/server/speaker.c b/server/speaker.c
index 407b1d7a6f37000e9d1334eadf8d5c0333dc881a..7411a8eff08e6475966712bb9cb06d74850a449a 100644 (file)
--- a/server/speaker.c
+++ b/server/speaker.c
@@ -553,21 +553,31 @@ static void mainloop(void) {
           report();
          break;
        case SM_CANCEL:
           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) {
          t = removetrack(sm.id);
          if(t) {
            if(t == playing) {

+              /* scratching the playing track */

               sm.type = SM_FINISHED;
               sm.type = SM_FINISHED;

-              strcpy(sm.id, playing->id);
-              speaker_send(1, &sm);

              playing = 0;
              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 {
            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;
             sm.type = SM_UNKNOWN;

-            speaker_send(1, &sm);

            error(0, "SM_CANCEL for unknown track %s", sm.id);
           }
            error(0, "SM_CANCEL for unknown track %s", sm.id);
           }

+          speaker_send(1, &sm);

           report();
          break;
        case SM_RELOAD:
           report();
          break;
        case SM_RELOAD: