[PR #64] [CLOSED] Fix/http client disposal #79

New issue

Closed

opened 2026-03-23 20:35:41 +00:00 by mirror · 0 comments

mirror commented

2026-03-23 20:35:41 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/tubearchivist/tubearchivist-jf-plugin/pull/64
Author: @mattkduran
Created: 10/18/2025
Status: ❌ Closed

Base: master ← Head: fix/http-client-disposal

📝 Commits (6)

ab561ab Fix: Implement IDisposable to properly clean up resources
f8231f1 Fix: Implement IDisposable to properly clean up resources
0533abb Update initialization
d9aaa8d Fixed loop
eb59a87 Fix resource leaks, crashes, and add comprehensive error handling
9b2ad42 Added doc strings to clarify complex logic for caching methods

📊 Changes

9 files changed (+1321 additions, -57 deletions)

View changed files

➕ .github/workflows/test.yaml (+19 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/CollectionMembershipTests.cs (+360 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/CollectionTests.cs (+307 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/GlobalUsings.cs (+1 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/Jellyfin.Plugin.TubeArchivistMetadata.Tests.csproj (+30 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/PluginTests.cs (+178 -0)
➕ Jellyfin.Plugin.TubeArchivistMetadata.Tests/UnitTest1.cs (+10 -0)
📝 Jellyfin.Plugin.TubeArchivistMetadata/Configuration/PluginConfiguration.cs (+41 -8)
📝 Jellyfin.Plugin.TubeArchivistMetadata/Plugin.cs (+375 -49)

📄 Description

Fix: Resource leaks and crashes when playing non-TubeArchivist media

This PR addresses multiple critical bugs that cause Jellyfin to crash and leak system resources.

Issues Fixed

Fixes #29 - Crash when playing Live TV (System.ArgumentException: Guid can't be empty)
Fixes #60 - Crash when playing movies outside TubeArchivist library
Fixes resource leak - HttpClient and HttpClientHandler were never disposed, causing memory/socket leaks

Root Cause Analysis

Crash Issues (#29, #60):
The plugin was attempting to traverse parent hierarchy for ALL media during playback, including Live TV and movies which have invalid/empty parent GUIDs. This caused LibraryManager.GetItemById(Guid id) to throw exceptions and crash Jellyfin.

Resource Leak:
The HttpClient and HttpClientHandler created in the Plugin constructor were never disposed, leading to:

Memory leaks
Socket exhaustion over time
Event handler subscriptions never cleaned up

Changes Made

1. Implement IDisposable Pattern

Plugin now properly implements IDisposable
Disposes HttpClient and HttpClientHandler on shutdown
Unsubscribes from PlaybackProgress and UserDataSaved events
Prevents memory leaks and resource exhaustion

2. Efficient Collection Validation with Caching

Cache TubeArchivist collection ID on plugin startup for O(1) lookups
Add IsItemInTubeArchivistCollection() method with:
- Early type checking - Reject non-Episode items immediately (fixes #29, #60)
- Cached collection ID matching - Efficient parent chain traversal
- Depth limiting - Prevent infinite loops (max 10 levels)
- Null safety - Handle broken hierarchies gracefully
Add RefreshTubeArchivistCollectionId() for configuration changes
Fallback to hierarchy-based checking when cache unavailable

3. Improved Event Handlers

OnPlaybackProgress and OnWatchedStatusChange now validate items BEFORE attempting hierarchy traversal
Added proper null checks and early exits
Better exception handling and logging

4. Comprehensive Test Suite

Added 23 tests covering:

Plugin lifecycle tests (8 tests)
- HttpClient initialization and disposal
- Event subscription and unsubscription
- Idempotent disposal
- Authorization header management
Collection caching tests (7 tests)
- Finding collections by name (case-insensitive)
- Handling missing collections
- Multiple collection scenarios
- Collection refresh functionality
Collection membership validation tests (8 tests)
- Type validation (non-Episodes rejected)
- Hierarchy traversal validation
- Cached vs fallback path testing
- Deep hierarchy handling
- Null parent handling

Technical Details

Before:

// No validation - crashes on Live TV/Movies
private async void OnPlaybackProgress(object? sender, PlaybackProgressEventArgs eventArgs)
{
    // ... code that assumes TubeArchivist media
    BaseItem? season = LibraryManager.GetItemById(item.ParentId); // CRASH: empty GUID
}

After:

// Validates type and collection membership first
private async void OnPlaybackProgress(object? sender, PlaybackProgressEventArgs eventArgs)
{
    // Early exit for non-TubeArchivist items
    if (!IsItemInTubeArchivistCollection(eventArgs.Item))
    {
        return; // Safe - no crash!
    }
    // ... rest of code only runs for valid TubeArchivist episodes
}

Performance Improvements

Collection validation: O(n) hierarchy traversal → O(1) cached lookup
Memory usage: Fixed resource leak from undisposed HttpClient
Crash prevention: Eliminated crashes from invalid media types

Breaking Changes

None - fully backward compatible.

Testing

All 23 tests pass:

Passed!  - Failed:     0, Passed:    23, Skipped:     0, Total:    23

Test infrastructure established for future contributions.

This PR supersedes and extends:

#62 - Initial Guid error fix
#63 - Collection caching implementation

Those PRs can be closed as this includes their fixes with additional improvements and comprehensive test coverage.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/tubearchivist/tubearchivist-jf-plugin/pull/64 **Author:** [@mattkduran](https://github.com/mattkduran) **Created:** 10/18/2025 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix/http-client-disposal` --- ### 📝 Commits (6) - [`ab561ab`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/ab561abee4d040377f62f5b083a5baafabb9f3cd) Fix: Implement IDisposable to properly clean up resources - [`f8231f1`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/f8231f164f3275480dc2275b00f2d4fb3050578b) Fix: Implement IDisposable to properly clean up resources - [`0533abb`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/0533abbed60d23c0af070d16c0f46af1b9332190) Update initialization - [`d9aaa8d`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/d9aaa8d19508afb2230d3a3859aa2e6d061162af) Fixed loop - [`eb59a87`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/eb59a87dac4dcccf9b4891b8e3c7403772e6076a) Fix resource leaks, crashes, and add comprehensive error handling - [`9b2ad42`](https://github.com/tubearchivist/tubearchivist-jf-plugin/commit/9b2ad426aa7e7522b26fc919ad78b2d77080bf32) Added doc strings to clarify complex logic for caching methods ### 📊 Changes **9 files changed** (+1321 additions, -57 deletions) <details> <summary>View changed files</summary> ➕ `.github/workflows/test.yaml` (+19 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/CollectionMembershipTests.cs` (+360 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/CollectionTests.cs` (+307 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/GlobalUsings.cs` (+1 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/Jellyfin.Plugin.TubeArchivistMetadata.Tests.csproj` (+30 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/PluginTests.cs` (+178 -0) ➕ `Jellyfin.Plugin.TubeArchivistMetadata.Tests/UnitTest1.cs` (+10 -0) 📝 `Jellyfin.Plugin.TubeArchivistMetadata/Configuration/PluginConfiguration.cs` (+41 -8) 📝 `Jellyfin.Plugin.TubeArchivistMetadata/Plugin.cs` (+375 -49) </details> ### 📄 Description ## Fix: Resource leaks and crashes when playing non-TubeArchivist media This PR addresses multiple critical bugs that cause Jellyfin to crash and leak system resources. ### Issues Fixed - **Fixes #29** - Crash when playing Live TV (`System.ArgumentException: Guid can't be empty`) - **Fixes #60** - Crash when playing movies outside TubeArchivist library - **Fixes resource leak** - HttpClient and HttpClientHandler were never disposed, causing memory/socket leaks ### Root Cause Analysis **Crash Issues (#29, #60):** The plugin was attempting to traverse parent hierarchy for ALL media during playback, including Live TV and movies which have invalid/empty parent GUIDs. This caused `LibraryManager.GetItemById(Guid id)` to throw exceptions and crash Jellyfin. **Resource Leak:** The `HttpClient` and `HttpClientHandler` created in the Plugin constructor were never disposed, leading to: - Memory leaks - Socket exhaustion over time - Event handler subscriptions never cleaned up ### Changes Made #### 1. Implement IDisposable Pattern - Plugin now properly implements `IDisposable` - Disposes `HttpClient` and `HttpClientHandler` on shutdown - Unsubscribes from `PlaybackProgress` and `UserDataSaved` events - Prevents memory leaks and resource exhaustion #### 2. Efficient Collection Validation with Caching - Cache TubeArchivist collection ID on plugin startup for O(1) lookups - Add `IsItemInTubeArchivistCollection()` method with: - **Early type checking** - Reject non-Episode items immediately (fixes #29, #60) - **Cached collection ID matching** - Efficient parent chain traversal - **Depth limiting** - Prevent infinite loops (max 10 levels) - **Null safety** - Handle broken hierarchies gracefully - Add `RefreshTubeArchivistCollectionId()` for configuration changes - Fallback to hierarchy-based checking when cache unavailable #### 3. Improved Event Handlers - `OnPlaybackProgress` and `OnWatchedStatusChange` now validate items BEFORE attempting hierarchy traversal - Added proper null checks and early exits - Better exception handling and logging #### 4. Comprehensive Test Suite Added 23 tests covering: - **Plugin lifecycle tests** (8 tests) - HttpClient initialization and disposal - Event subscription and unsubscription - Idempotent disposal - Authorization header management - **Collection caching tests** (7 tests) - Finding collections by name (case-insensitive) - Handling missing collections - Multiple collection scenarios - Collection refresh functionality - **Collection membership validation tests** (8 tests) - Type validation (non-Episodes rejected) - Hierarchy traversal validation - Cached vs fallback path testing - Deep hierarchy handling - Null parent handling ### Technical Details **Before:** ``` // No validation - crashes on Live TV/Movies private async void OnPlaybackProgress(object? sender, PlaybackProgressEventArgs eventArgs) { // ... code that assumes TubeArchivist media BaseItem? season = LibraryManager.GetItemById(item.ParentId); // CRASH: empty GUID } ``` **After:** ``` // Validates type and collection membership first private async void OnPlaybackProgress(object? sender, PlaybackProgressEventArgs eventArgs) { // Early exit for non-TubeArchivist items if (!IsItemInTubeArchivistCollection(eventArgs.Item)) { return; // Safe - no crash! } // ... rest of code only runs for valid TubeArchivist episodes } ``` ### Performance Improvements - **Collection validation**: O(n) hierarchy traversal → O(1) cached lookup - **Memory usage**: Fixed resource leak from undisposed HttpClient - **Crash prevention**: Eliminated crashes from invalid media types ### Breaking Changes None - fully backward compatible. ### Testing All 23 tests pass: ``` Passed! - Failed: 0, Passed: 23, Skipped: 0, Total: 23 ``` Test infrastructure established for future contributions. ### Related PRs This PR supersedes and extends: - #62 - Initial Guid error fix - #63 - Collection caching implementation Those PRs can be closed as this includes their fixes with additional improvements and comprehensive test coverage. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>