Podcasts: Pre-caching image and more-info data can bring the server to a crawl

[mw9]

These proposed changes seek to address issues that have arisen with pre-caching Podcast feed images and data.

At present, there is a problem with the pre-caching process, because it potentially generates a large number of simultaneous feed requests which can overload a modestly powered server, especially where the user subscribes to a large number of feeds. It's not an obvious problem on, say, an iMac, but it has created problems on a Raspberry Pi, as reported here: Slow loading of a list of podcast episodes.

I have submitted the PR against LMS 8.2, on the premise that the issue is a bug in 8.2. But it can be applied equally well to 8.3.

The proposed changes are:

• Podcast: Pre-caching - Guard against fetching same feed multiple times
  This change ensures that only one pre-caching attempt per day will be made for each feed. At present, a pre-caching attempt may be made on each occasion that the user browses within the podcast menu.

• Podcast: Pre-caching - Randomize image & moreInfo caching period
  This change implements randomization of the cache period, to reduce the likelihood that future cache refreshes will happen all at once. It also sets the cache period for a podcast's 'moreInfo' data to the same period as the podcast image. At present that data is only cached for the default period of one hour, which can become burdensome if the user has many feeds.

• Podcast: Pre-caching - Refresh pre-cached data while browsing a feed
  This change implements an opportunistic refresh of the cache when the user browses into a podcast feed. It will help to keep the cache up to date.

• Podcast: Pre-caching - Schedule activity over an extended period
  This change spreads out the pre-caching activity by launching each feed at 30 second intervals. Lower powered servers will benefit from this somewhat laid back approach. It does mean that a user with a large number of feeds will wait longer to see the podcast images properly populated, but perhaps that is not so very important.
  Update: Commit now revised - refer below.

[philippe44]

You're right about the load on more simple servers and the need to not query the same thing multiple times, I often forget that LMS redescend XML tree multiple time over navigation, but don't you think that something basic like that would be good enough

sub handleFeed {
	my ($client, $cb, $params, $args) = @_;

	my $items = [];
	my $provider = getProviderByName();

	# populate provider's custom menu
	foreach my $item (@{$provider->getMenuItems($client)}) {
		$item->{name}  ||= cstring($client, 'PLUGIN_PODCAST_SEARCH');
		$item->{type}  ||= 'search';
		$item->{url}   ||= \&searchHandler unless $item->{enclosure};
		$item->{passthrough} ||= [ { provider => $provider, item => $item } ];

		if (!$item->{image} || ref $item->{image}) {
			$item->{image} = 'html/images/search.png';
		}

		push @$items, $item;
	}

	# then add recently played
	push @$items, {
		name  => cstring($client, 'PLUGIN_PODCAST_RECENTLY_PLAYED'),
		url   => \&recentHandler,
		type  => 'link',
		image => __PACKAGE__->_pluginDataFor('icon'),
	};

	# then existing feeds
	my @feeds = @{$prefs->get('feeds')};
	my @need;
	my $fetch;
	
	foreach ( @feeds ) {
		my $url = $_->{value};
		my $image = $cache->get('podcast-rss-' . $url);

		push @$items, {
			name => $_->{name},
			url  => $url,
			favorites_url => $url,
			favorites_type => 'link',
			parser => 'Slim::Plugin::Podcast::Parser',
			image => $image || __PACKAGE__->_pluginDataFor('icon'),
			playlist => $url,
		};

		unless ($image && $cache->get('podcast_moreInfo_' . $url)) {
			$cache->set('podcast-rss-' . $url, __PACKAGE__->_pluginDataFor('icon'), '1days');
			$cache->set('podcast_moreInfo_' . $url, {}, '1days');
			push (@need, $url);
		}	
	}

	$fetch = sub {
		my $url = pop @need;
		Slim::Formats::XML->getFeedAsync(
			sub {
				_precacheShowDetails($url, $_[0]);
				$fetch->();
			},
			sub {
				$log->warn("can't get $url RSS feed information: ", $_[0]);
				$fetch->();
			},
			{
				parser => 'Slim::Plugin::Podcast::Parser',
				url => $url,
				expires => 86400,
			}
		) if $url;
	};
	
	# get missing cache images & moreinfo if any
	$fetch->();
	
	$cb->({
		items => $items,
		actions => {
			info => {
				command   => ['podcastinfo', 'items'],
				variables => [ 'url', 'url', 'name', 'name', 'image', 'image' ],
			},
		}
	});
}

I'm not a big fan of timers in general and this alternative simply serialize the requests, so there is no flooding of 100's and when descending a tree, grabbing the episodes of a single feed will simply enter into concurrence with just one caching request. It's faster (no need to wait 30s) when the opportunity permits, while not being in the way of other user-requested browsing.

[michaelherger]

Thanks @mw9 for identifying this issue - good catch! I like the idea of the randomized TTL for the cache and the update whenever available.

But I'd agree with @philippe44 about the timers: they're always wrong - too short or too long, but never right 😀. I mentioned some modules in the forum thread, but @philippe44's approach seems good enough to me. The Async::Util::amap() would have had a slight advantage in that it would allow for some level of parallelism without opening too many connections at the same time. But that's optimization.

Could you please factor in @philippe44's suggestion to get rid of the timer? The rest looks good to me. Thanks again!

[mw9]

I have taken on board your discomfort with my laid back timers 😀, and have taken up @philippe44's elegant suggestion in its place.

It seems to work fine on my modestly powered server.

Just the last commit is changed:

• Podcast: Pre-caching - Schedule activity over an extended period
  This change spreads out the pre-caching activity by fetching and parsing each feed sequentially. Lower powered servers will benefit from this approach, because the server will only be processing one feed at a time, instead of processing all feeds concurrently.

[mw9]

A remark:

While playing around with this last week, I ran across a curious issue. It seems that, if there is an http failure, there are circumstances in which Slim::Formats::XML->getFeedAsync can call its error callback twice. For the same feed.

But the remote http server that was triggering the problem has now started to behave itself, and I have not been able to reproduce the circumstances. At first I thought it might be something to do with Async::Util::amap(). But it isn't. Perhaps, one day, I shall track it down.

There is no particularly adverse effect on this proposed change. It just means that we would be triggered into fetching the next feed a little earlier than anticipated. It will probably be a rare occurrence.

[mherger]

Thanks for the update. I was wondering whether you didn't pre-cache things twice now: once in the parser, then in the callback triggered with the parser's result?

[mherger]

Thanks for this PR! I merged it to 8.3 for now. Should it turn out to be good, I'll back-port to 8.2. Please give it some testing.

[mw9]

Thanks for this PR! I merged it to 8.3 for now. Should it turn out to be good, I'll back-port to 8.2. Please give it some testing.

Forum user @Aphonia, who raised the issue, has provided me with his list of (125) podcasts that were causing him pain.
I have replicated his pain on my own (low powered) system, and confirmed that these changes do seem to resolve the issues.

One matter has arisen:

It is quite acceptable for a podcast not to provide an image. In this case we provide a default image, but only for 24 hours. Thereafter we just fetch the feed again, and re-try. That seems, to me, to be somewhat unnecessary, and perhaps "rude". I shall propose in a follow up PR that we adopt the same circa 90 day policy that is applied to "real" images.

I also noted some small changes that might be made to the XML/RSS feed parser, which I will offer up in a follow up PR.