rework idling again, walking around Net::Async::MPD interface

thing is, that the ->noidle() call places readers on the mpd handle,
because it uses ->send(). this interferes with the expected protocol
flow like this:

idle() is called, a reader/parser is queued by ->send()
when noidle is called, it calls send(), which queues another reader/parser
MPD sees the 'noidle' command and responds with plain 'OK', which is
consumed by the readers queued by idle(). this by itself causes a crash
because the is no "result" and ->{changes} is invoked on undefined value

it the undef deref is fixed, the protocol still hangs, because of the
extra readers queued, which consumes the reply of the next command
issued (e.g. 'playlist')

the other thing that doesn't work is the link check in mpd-feeder. on
timer, it wants to break out of 'idle' waiting just to be sure that the
connection is alive. this also causes dereferencing of an undefined
value and protocol lock up.

so, instead, the idle/noidle is implemented half under-the-table.
idle is implemented via ordinary send(), and broken by a crude call to
mpd->{mpd_handle}->write(), breaking API. this serves two purposes.
first, the readers queued by send() see toe 'OK' resulting from noidle
and return empty result, which is handled by Feeder without crashing.
second, sending a low-level 'noidle' command via the connection handle
avoids queuing extra reader which would cause protocol hang.

the result is that periodic connection checks (leaving and re-entering
idle) work, idle events (which put MPD out of idle mode) are handled
outside of 'idleness' as they should be, asynchronous signals
(TERM, INT, etc.) work as expected outside of 'idleness'. happy camping
all around!

diff --git a/lib/App/MPD/Feeder.pm b/lib/App/MPD/Feeder.pm
index 2ce166f6a4b45e1a4a5534534af2a474eafe2bd9..60b54a56b7a33c8e563dfbc7f8c6130091bc1f72 100644 (file)
--- a/lib/App/MPD/Feeder.pm
+++ b/lib/App/MPD/Feeder.pm
@@ -5,8 +5,9 @@ use warnings;
 use utf8;
 use feature 'state';
 
-use App::MPD::Feeder::Options;
 use App::MPD::Feeder::DB;
+use App::MPD::Feeder::Options;
+use App::MPD::Feeder::WorkQueue;
 use DBD::Pg;
 use DBI;
 use Getopt::Long;
@@ -17,7 +18,6 @@ use Net::Async::MPD;
 use Object::Pad;
 use Syntax::Keyword::Try;
 
-
 class App::MPD::Feeder {
     has $cfg_file :reader;
     has $opt :reader;
@@ -25,6 +25,8 @@ class App::MPD::Feeder {
     has $db_needs_update :writer = 1;
     has $mpd :reader;
     has $idler;
+    has $work_queue = App::MPD::Feeder::WorkQueue->new;
+    has $last_mpd_comm;
 
 use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
 
@@ -68,12 +70,23 @@ use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
                 die "Connection to MPD lost";
             }
         );
+        $mpd->on(
+            playlist => sub {
+                $work_queue->add('playlist');
+            }
+        );
+        $mpd->on(
+            database => sub {
+                $work_queue->add('database');
+            }
+        );
 
         my $int_signal_handler = sub {
             state $signal_count = 0;
             $signal_count++;
             $log->debug("Signal received. Stopping loop");
-            $mpd->loop->stop('quit');
+            $work_queue->add('quit');
+            $self->break_idle;
 
             if ( $signal_count > 1 ) {
                 $log->warn("Another signal received (#$signal_count)");
@@ -96,7 +109,8 @@ use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
                 name       => 'HUP',
                 on_receipt => sub {
                     $log->debug("SIGHUP received. Scheduling reload");
-                    $mpd->loop->stop('reload');
+                    $work_queue->add('reload');
+                    $self->break_idle;
                 },
             )
         );
@@ -170,25 +184,20 @@ use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
         }
     }
 
-    method queue_songs($num = undef, $callback = undef) {
+    method queue_songs($num = undef) {
         $self->connect_db;
         if (!defined $num) {
             $self->connect_mpd;
-            $mpd->send('playlist')->on_done(
-                sub {
-                    my $present = scalar @{ $_[0] // [] };
-
-                    $log->notice( "Playlist contains $present songs. Wanted: "
-                            . $opt->target_queue_length );
-                    if ( $present < $opt->target_queue_length ) {
-                        $self->queue_songs(
-                            $opt->target_queue_length - $present, $callback );
-                    }
-                    else {
-                        $callback->() if $callback;
-                    }
-                }
-            );
+            $log->trace("Requesting playlist");
+            my $present = $mpd->send('playlist')->get // [];
+            $present = scalar(@$present);
+
+            $log->notice( "Playlist contains $present songs. Wanted: "
+                    . $opt->target_queue_length );
+            if ( $present < $opt->target_queue_length ) {
+                $self->queue_songs(
+                    $opt->target_queue_length - $present );
+            }
 
             return;
         }
@@ -228,47 +237,58 @@ use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
         $f->on_done(
             sub {
                 $self->db->note_song_qeued($_) for @list;
-                $callback->(@_) if $callback;
             }
         );
+        $f->get;
     }
 
-    method prepare_to_wait_idle {
-        $log->trace('declaring idle mode');
-        $idler = $mpd->send('idle database playlist')->on_done(
-            sub {
-                my $result = shift;
+    method stop {
+        undef $mpd;
 
-                undef $idler;
+        $db->disconnect;
+    }
 
-                my $changed = $result->{changed} // '';
+    method handle_work_queue {
+        while ( my $item = $work_queue->next ) {
+            if ( $item eq 'playlist' ) {
+                $self->queue_songs;
+            }
+            elsif ( $item eq 'database' ) {
+                $db_needs_update = 1;
+                $self->update_db;
+            }
+            elsif ( $item eq 'reload' ) {
+                $log->notice("disconnecting and re-starting");
+                $self->stop;
 
-                if ( $changed eq 'database' ) {
-                    $db_needs_update = 1;
-                    $self->prepare_to_wait_idle;
-                }
-                elsif ( $changed eq 'playlist' ) {
-                    $self->queue_songs( undef,
-                        sub { $self->prepare_to_wait_idle } );
-                }
-                elsif ( $changed eq '' ) {
-                    $log->debug("got no changes from idle");
-                    $self->prepare_to_wait_idle;
-                }
-                else {
-                    use JSON;
-                    $log->warn(
-                        "Unknown result from idle: " . to_json($result) );
-                    $self->prepare_to_wait_idle;
+                my @exec =
+                    ( $0, '--config', $self->cfg_file, '--skip-db-update' );
+                if ( $log->is_trace ) {
+                    $log->trace( 'exec '
+                            . join( ' ', map { /\s/ ? "'$_'" : $_ } @exec ) );
                 }
+                exec(@exec);
             }
-        );
+            elsif ( $item eq 'quit' ) {
+                $log->trace("quitting");
+                $self->stop;
+                exit 0;
+            }
+            else {
+                die "Unknown work queue item '$item'";
+            }
+        }
     }
 
-    method stop {
-        undef $mpd;
-
-        $db->disconnect;
+    method break_idle {
+        if ($idler && !$idler->is_ready) {
+            $log->trace("hand-sending 'noidle'");
+            undef $idler;
+            $mpd->{mpd_handle}->write("noidle\n");;
+        }
+        else {
+            $log->trace("no idler found");
+        }
     }
 
     method run_loop {
@@ -277,43 +297,42 @@ use constant DEFAULT_CONFIG_FILE => '/etc/mpd-feeder/mpd-feeder.conf';
 
         $mpd->loop->add(
             IO::Async::Timer::Periodic->new(
-                interval => 300,
+                interval => 60,
                 on_tick  => sub {
-                    if ($idler) {
-                        $log->trace('breaking idle to see if MPD is there');
-                        undef $idler;
-                        $log->trace("> noidle (direct)");
-                        $mpd->{mpd_handle}->write("noidle\n");
+                    if ( time - $last_mpd_comm > 300 ) {
+
+                        $log->trace(
+                            "no active MPD communication for more that 5 minutes"
+                        );
+                        $log->trace("forcing alive check");
+                        $self->break_idle;
+                    }
+                    else {
+                        $log->trace("contacted MPD less than 5 minutes ago. skipping alive check");
                     }
                 },
             )->start
         );
 
-        for ( ;; ) {
-            $self->queue_songs( undef, sub { $self->prepare_to_wait_idle } );
+        $self->queue_songs;
 
-            $log->debug("Entering event loop. PID=$$");
+        for ( ;; ) {
+            $log->debug("Waiting idle. PID=$$");
+            $last_mpd_comm = time;
+            $idler = $mpd->send("idle database playlist");
+            my $result = $idler->get;
+            undef $idler;
 
-            my $result = $mpd->loop->run;
-            $log->trace( "Got loop result of " . ( $result // 'undef' ) );
+            if ($result and $result->{changed}){
+                my $changed = $result->{changed};
+                $changed = [ $changed ] unless ref $changed;
 
-            if ( 'reload' eq $result ) {
-                $log->notice("disconnecting");
-                $self->stop;
-
-                my @exec = ( $0, '--config', $self->cfg_file, '--skip-db-update' );
-                if ( $log->is_trace ) {
-                    $log->trace( 'exec '
-                            . join( ' ', map { /\s/ ? "'$_'" : $_ } @exec ) );
-                }
-                exec(@exec);
+                $mpd->emit($_) for @$changed;
             }
 
-            if ( 'quit' eq $result ) {
-                $log->trace("quitting because of 'quit' loop result");
-                $self->stop;
-                exit 0;
-            }
+            $log->trace('got out of idle');
+
+            $self->handle_work_queue;
         }
     }
 }