From 42423f2c69f3d70cc161333175bd217d8a9b7594 Mon Sep 17 00:00:00 2001 From: Damyan Ivanov Date: Sun, 21 Nov 2021 08:44:35 +0000 Subject: [PATCH] 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! --- lib/App/MPD/Feeder.pm | 171 +++++++++++++++++++++++------------------- 1 file changed, 95 insertions(+), 76 deletions(-) diff --git a/lib/App/MPD/Feeder.pm b/lib/App/MPD/Feeder.pm index 2ce166f..60b54a5 100644 --- 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; } } } -- 2.39.5