Sync vs. async web calls

[michaelherger]

Hi @sspiff - congrats on a nice plugin! I'm glad somebody picked this up, as it helps with one of the big issues many Radio users have with the MySB shutdown.

Here's more of a hint than an issue: I understand you're using an existing library to interact with the service. This comes with a potential "risk": it's using sync (blocking) HTTP requests (LWP::UserAgent). Most of the time this is perfectly fine. BUT if the service doesn't respond in a timely manner, it could potentially interrupt playback and lock up LMS until the timeout kicks in.

I haven't checked your code, but I'd suggest you keep the timeout aggressively low (eg. 5s) in order to minimise that risk. Or you leave it as it is, but keep in mind this could cause some feedback from the users. Maybe it's a once in a few weeks issue.

I fully understand that re-writing that module to use LMS' own async calls, the JSON module that comes with etc. would be a daunting task.

Thanks again for your support of the community!

[sspiff]

Yeah, I started to convert the existing library (or re-write it), but realized it was going to take me a while and I wanted to get the plugin out before the end of the month. In practice, the remote service seems to be pretty quick, but moving to async io is probably a good idea. Perhaps @nabertrand and I can collaborate on this.

Looks like the default timeout used by the existing lib is 10s.

Is there any documentation or a good example for using the LMS async io?

[michaelherger]

"Documentation" is none of LMS' strength... but there actually is a bit of documentation for this in the code!

https://github.com/Logitech/slimserver/blob/public/8.4/Slim/Networking/SimpleAsyncHTTP.pm#L338

I guess you already got that you won't be able to just call it and wait for the result, but you'll have to pass a callback that then would be called once the request had succeeded (or failed). This often requires a good level of code re-org.

Maybe you can concentrate on what is being used most often, and while actually playing music - which likely will be the next track lookup? One recent example would be the implementation Philippe did here:

https://github.com/michaelherger/lms-plugin-tidal/blob/main/ProtocolHandler.pm#L125

[nabertrand]

@sspiff I believe all of the API calls in use in this plugin have been converted to use Slim::Networking::SimpleAsyncHTTP instead of LWP::UserAgent / HTTP::Request in https://github.com/nabertrand/lms-plugin-pandoradirect/tree/main/lib/WebService. I created a fork of this repo so you can see the changes: https://github.com/sspiff/lms-plugin-pyrrha/compare/main...nabertrand:lms-plugin-pyrrha:async?expand=1

Once you update the WebService module to use asynchronous calls, the main change to your code would then be providing a callback code ref to the API calls. Here's an example from my plugin: https://github.com/nabertrand/lms-plugin-pandoradirect/blob/0c56d73b75ce617c2629d4d1af78c1036ee74cd2/Plugin.pm#L84-L98 And an example of changing your login code:

diff --git a/plugin/Utils.pm b/plugin/Utils.pm
index a6fe997..1787892 100644
--- a/plugin/Utils.pm
+++ b/plugin/Utils.pm
@@ -43,21 +43,23 @@ sub getWebService {
               partner  => WebService::Pandora::Partner::AIR->new(),
               expiresAt => time() + $WEBSVC_LIFETIME,
             );
-  if (!$websvc->login()) {
-    my $e = $websvc->error();
-    if (ref $e eq 'HASH') {
-      $e = $e->{'message'}
-    }
-    $log->error($e);
-    $errorCb->($e);
-    return;
-  }
-  $log->info('login successful');
+  $websvc->login(
+    sub {
+      if (my $e = $websvc->error()) {
+       if (ref $e eq 'HASH') {
+          $e = $e->{'message'}
+        }
+        $log->error($e);
+        $errorCb->($e);
+        return;
+      }
+      $log->info('login successful');

-  $cache{'webService'} = $websvc;
+      $cache{'webService'} = $websvc;

-  $successCb->($websvc);
-  return;
+      $successCb->($websvc);
+    }
+  );
 }

[sspiff]

Thanks @nabertrand! I'll starting digging into this today.

[sspiff]

asyncio branch is tracking this: https://github.com/sspiff/lms-plugin-pyrrha/tree/asyncio

[michaelherger]

Nice job @nabertrand & @sspiff !