Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix long live tv load times, Fixes #10761 #10881

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

TelepathicWalrus
Copy link
Contributor

Second attempt at fixing this issue, this time i actually read the logs! Adds an extra test to check that the cached media info file can be read. This allows the rest of the function to run if the file doesn't exist.

Changes
Add extra try/catch to stop errors if trying to read cached media info file that doesn't exist

Issues
Fixes #10761

@TelepathicWalrus TelepathicWalrus changed the title Add ex to catch if cached mediainfo doesnt exist Fix long live tv load times, Fixes #10761 Jan 17, 2024
@TelepathicWalrus
Copy link
Contributor Author

In Emby.Server.Implementations/Library/MediaSourceManager.cs there is the same function with the same AsyncFile.OpenRead(cacheFilePath) that caused issues here. Would it be a good idea to change that too? If so I can update the pull request here and do both at the same time or I can create a separate pull request. If not then I'll leave it 👍

Comment on lines 55 to 65
try
{
mediaInfo = await JsonSerializer.DeserializeAsync<MediaInfo>(jsonStream, _jsonOptions, cancellationToken).ConfigureAwait(false);
// _logger.LogDebug("Found cached media info");
}
catch (Exception ex)
{
_logger.LogError(ex, "Error deserializing mediainfo cache");
}

await jsonStream.DisposeAsync().ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try
{
mediaInfo = await JsonSerializer.DeserializeAsync<MediaInfo>(jsonStream, _jsonOptions, cancellationToken).ConfigureAwait(false);
// _logger.LogDebug("Found cached media info");
}
catch (Exception ex)
{
_logger.LogError(ex, "Error deserializing mediainfo cache");
}
await jsonStream.DisposeAsync().ConfigureAwait(false);
await using (jsonStream.ConfigureAwait(false))
{
mediaInfo = await JsonSerializer.DeserializeAsync<MediaInfo>(jsonStream, _jsonOptions, cancellationToken).ConfigureAwait(false);
// _logger.LogDebug("Found cached media info");
}

IMO converting the nested try-catch into a using statement makes more sense. I believe the new disposable analyzer would flag this, so we'd need to do it eventually anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, just wasn't sure if the DisposeAsync line was needed but i assume the using will dispose of it even if this fails. This does look much nicer than nested try catch. Ive updated PR.

await using (jsonStream.ConfigureAwait(false))
{
mediaInfo = await JsonSerializer.DeserializeAsync<MediaInfo>(jsonStream, _jsonOptions, cancellationToken).ConfigureAwait(false);
// _logger.LogDebug("Found cached media info");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the commented logDebug? If so it was there before so I left it there.

@Bond-009 Bond-009 merged commit a2fdec4 into jellyfin:master Feb 1, 2024
23 checks passed
KrzaQ pushed a commit to KrzaQ/jellyfin that referenced this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue]: LiveTV taking 200+ seconds to start - analyzeduration regression?
5 participants