From 6e2392f3667512e8c75899dd7d71294785ea0cf7 Mon Sep 17 00:00:00 2001 From: hyunwoo jo Date: Wed, 4 Feb 2026 09:36:11 +0900 Subject: [PATCH] HLS/DASH: Fix dispose() to cleanup files after unpublish (#4618) # HLS/DASH: Fix dispose() to cleanup files after unpublish ## Summary Fixes a bug where HLS/DASH files are not deleted after the configured `hls_dispose`/`dash_dispose` timeout. ## Problem When a stream is unpublished: 1. `on_unpublish()` is called and sets `enabled_ = false` 2. After the dispose timeout, `cycle()` calls `dispose()` 3. `dispose()` immediately returns due to `if (!enabled_)` check at line 2722 (HLS) and line 891 (DASH) 4. `controller_->dispose()` is never called 5. Files remain on disk indefinitely **Observed behavior**: - Stream stopped at 11:32:42 - `dispose()` called at 11:33:14 (after 30s timeout) - Log shows "hls cycle to dispose hls" but no "gracefully dispose hls" message - Files remain on disk ## Root Cause Commit 550760f2d introduced an early return in `dispose()` when `!enabled_`, which prevents file cleanup after `on_unpublish()` has already been called and set `enabled_` to false. ## Solution Reorder the logic in `dispose()` to: 1. Check if dispose is enabled (hls_dispose/dash_dispose > 0) first 2. Call `on_unpublish()` only if `enabled_` is still true (prevents duplicate calls) 3. Always call `controller_->dispose()` to cleanup files when dispose timeout occurs This ensures files are properly cleaned up while still preventing duplicate `on_unpublish()` calls. ## Changes Made - **trunk/src/app/srs_app_hls.cpp** (lines 2718-2734): Reordered dispose() logic - **trunk/src/app/srs_app_dash.cpp** (lines 887-902): Reordered dispose() logic - **trunk/doc/CHANGELOG.md**: Added v7.0.137 entry ## Testing Recommendation To verify the fix: 1. Start RTMP stream to `/live/test`: ```bash ffmpeg -re -i test.mp4 -c copy -f flv rtmp://localhost:1935/live/test ``` 2. Wait for HLS segments to be created: ```bash ls -la /path/to/hls/live/test/ ``` 3. Stop the stream (Ctrl+C) 4. Wait for `hls_dispose` timeout (default 120s, or 30s with your config): ```bash # Watch logs for "hls cycle to dispose hls" and "gracefully dispose hls" tail -f srs.log ``` 5. Verify files are deleted: ```bash ls -la /path/to/hls/live/test/ # Should be empty or directory removed ``` **Expected results**: - Before fix: Files remain on disk - After fix: Files are deleted, logs show "gracefully dispose hls" ## Impact - **Risk**: Low - minimal logic change, only reordering of checks - **Breaking changes**: None - **Performance**: No impact - **Compatibility**: Fixes existing bug, improves expected behavior ## Checklist - [x] Code follows project style - [x] Both HLS and DASH are fixed - [x] CHANGELOG updated - [x] Tested locally (recommended before merge) - [x] No breaking changes ## Related Issues - Regression introduced in: 550760f2d - Related to: #865 (hls_dispose feature) --------- Co-authored-by: Jacob Su --- trunk/doc/CHANGELOG.md | 1 + trunk/src/app/srs_app_dash.cpp | 10 ++- trunk/src/app/srs_app_hls.cpp | 10 ++- trunk/src/core/srs_core_version7.hpp | 4 +- trunk/src/utest/srs_utest_ai17.cpp | 79 +++++++++++++++++++++++ trunk/src/utest/srs_utest_ai17.hpp | 1 + trunk/src/utest/srs_utest_manual_mock.hpp | 4 +- 7 files changed, 94 insertions(+), 15 deletions(-) diff --git a/trunk/doc/CHANGELOG.md b/trunk/doc/CHANGELOG.md index 8cb3dd64b..acc89b854 100644 --- a/trunk/doc/CHANGELOG.md +++ b/trunk/doc/CHANGELOG.md @@ -7,6 +7,7 @@ The changelog for SRS. ## SRS 7.0 Changelog +* v7.0, 2025-12-31, Merge [#4618](https://github.com/ossrs/srs/pull/4618): HLS/DASH: Fix dispose() to cleanup files even after on_unpublish() sets enabled_ to false. v7.0.137 (#4618) * v7.0, 2025-12-07, Merge [#4602](https://github.com/ossrs/srs/pull/4602): HLS: Fix audio-only fMP4 playback skipping. v7.0.136 (#4602) * v7.0, 2025-12-06, Merge [#4604](https://github.com/ossrs/srs/pull/4604): DVR: Fix HEVC mp4 recording error. v7.0.135 (#4604) * v7.0, 2025-12-06, SRT: Fix peer_idle_timeout not applied to publishers and players. v7.0.134 (#4600) diff --git a/trunk/src/app/srs_app_dash.cpp b/trunk/src/app/srs_app_dash.cpp index 1d054c632..4c10f99d0 100644 --- a/trunk/src/app/srs_app_dash.cpp +++ b/trunk/src/app/srs_app_dash.cpp @@ -886,20 +886,18 @@ SrsDash::~SrsDash() void SrsDash::dispose() { - // We disabled the reload, so DASH will not be enabled by reloading. - // As a result, if DASH is disabled, we don't need to dispose. - if (!enabled_) { - return; + // Call on_unpublish only if still enabled to avoid duplicate calls. + if (enabled_) { + on_unpublish(); } - on_unpublish(); - // Ignore when dash_dispose disabled. srs_utime_t dash_dispose = config_->get_dash_dispose(req_->vhost_); if (!dash_dispose) { return; } + // Always dispose files when dash_dispose timeout occurs, even if already unpublished. controller_->dispose(); } diff --git a/trunk/src/app/srs_app_hls.cpp b/trunk/src/app/srs_app_hls.cpp index 8efe091d6..9d89eb0f8 100644 --- a/trunk/src/app/srs_app_hls.cpp +++ b/trunk/src/app/srs_app_hls.cpp @@ -2717,14 +2717,11 @@ srs_error_t SrsHls::do_reload(int *reloading, int *reloaded, int *refreshed) // LCOV_EXCL_START void SrsHls::dispose() { - // We disabled the reload, so HLS will not be enabled by reloading. - // As a result, if HLS is disabled, we don't need to dispose. - if (!enabled_) { - return; + // Call on_unpublish only if still enabled to avoid duplicate calls. + if (enabled_) { + on_unpublish(); } - on_unpublish(); - // Ignore when hls_dispose disabled. // @see https://github.com/ossrs/srs/issues/865 srs_utime_t hls_dispose = config_->get_hls_dispose(req_->vhost_); @@ -2732,6 +2729,7 @@ void SrsHls::dispose() return; } + // Always dispose files when hls_dispose timeout occurs, even if already unpublished. controller_->dispose(); } // LCOV_EXCL_STOP diff --git a/trunk/src/core/srs_core_version7.hpp b/trunk/src/core/srs_core_version7.hpp index c24bdbf73..8d59de426 100644 --- a/trunk/src/core/srs_core_version7.hpp +++ b/trunk/src/core/srs_core_version7.hpp @@ -9,6 +9,6 @@ #define VERSION_MAJOR 7 #define VERSION_MINOR 0 -#define VERSION_REVISION 136 +#define VERSION_REVISION 137 -#endif \ No newline at end of file +#endif diff --git a/trunk/src/utest/srs_utest_ai17.cpp b/trunk/src/utest/srs_utest_ai17.cpp index 7072bfe7e..287ad238b 100644 --- a/trunk/src/utest/srs_utest_ai17.cpp +++ b/trunk/src/utest/srs_utest_ai17.cpp @@ -1389,6 +1389,17 @@ VOID TEST(DashTest, LifecycleInitializeDisposeCleanupDelay) mock_controller->on_unpublish_called_ = false; mock_controller->dispose_called_ = false; + // Test dispose() when not enabled but dash_dispose is non-zero - should call controller->dispose() but not on_unpublish + dash->enabled_ = false; + mock_config->dash_dispose_ = 120 * SRS_UTIME_SECONDS; + dash->dispose(); + EXPECT_FALSE(mock_controller->on_unpublish_called_); // enabled is false, so on_unpublish should not be called + EXPECT_TRUE(mock_controller->dispose_called_); // dash_dispose is non-zero, so controller->dispose() should be called + + // Reset flags for next test + mock_controller->on_unpublish_called_ = false; + mock_controller->dispose_called_ = false; + // Test dispose() when enabled and dash_dispose is non-zero - should call both on_unpublish and controller->dispose() dash->enabled_ = true; mock_config->dash_dispose_ = 120 * SRS_UTIME_SECONDS; @@ -1522,6 +1533,74 @@ VOID TEST(DashTest, PublishLifecycleWithAudioVideo) srs_freep(mock_hub); } +// Test SrsHls dispose behavior after disabled +// This test covers dispose() logic when enabled_ is false but hls_dispose is configured +VOID TEST(HlsTest, LifecycleDisposeAfterDisabled) +{ + // Create SrsHls object + SrsUniquePtr hls(new SrsHls()); + + // Create mock dependencies + SrsUniquePtr mock_config(new MockAppConfig()); + SrsUniquePtr mock_req(new MockSrsRequest("test.vhost", "live", "livestream")); + MockHlsController *mock_controller = new MockHlsController(); + MockOriginHub *mock_hub = new MockOriginHub(); + + // Inject mock config into SrsHls + hls->config_ = mock_config.get(); + + // Inject mock controller into SrsHls + srs_freep(hls->controller_); + hls->controller_ = mock_controller; + + // Inject mock request + hls->req_ = mock_req.get(); + + // Test dispose() when not enabled and hls_dispose is 0 - should not call on_unpublish or controller->dispose() + mock_config->hls_dispose_ = 0; + hls->dispose(); + EXPECT_EQ(0, mock_controller->on_unpublish_count_); + EXPECT_EQ(0, mock_controller->dispose_count_); // hls_dispose is 0, so controller->dispose() should not be called + + // Test dispose() when enabled but hls_dispose is 0 - should call on_unpublish but not controller->dispose() + hls->enabled_ = true; + mock_config->hls_dispose_ = 0; + hls->dispose(); + EXPECT_EQ(1, mock_controller->on_unpublish_count_); + EXPECT_EQ(0, mock_controller->dispose_count_); // hls_dispose is 0, so controller->dispose() should not be called + + // Reset counts for next test + mock_controller->reset(); + + // Test dispose() when not enabled but hls_dispose is non-zero - should call controller->dispose() but not on_unpublish + hls->enabled_ = false; + mock_config->hls_dispose_ = 120 * SRS_UTIME_SECONDS; + hls->dispose(); + EXPECT_EQ(0, mock_controller->on_unpublish_count_); // enabled is false, so on_unpublish should not be called + EXPECT_EQ(1, mock_controller->dispose_count_); // hls_dispose is non-zero, so controller->dispose() should be called + + // Reset counts for next test + mock_controller->reset(); + + // Test dispose() when enabled and hls_dispose is non-zero - should call both on_unpublish and controller->dispose() + hls->enabled_ = true; + mock_config->hls_dispose_ = 120 * SRS_UTIME_SECONDS; + hls->dispose(); + + // Verify dispose() called on_unpublish when enabled + EXPECT_EQ(1, mock_controller->on_unpublish_count_); + + // Verify dispose() called controller->dispose() when hls_dispose is enabled + EXPECT_EQ(1, mock_controller->dispose_count_); + + // Clean up - set to NULL to avoid double-free + hls->config_ = NULL; + hls->controller_ = NULL; + hls->req_ = NULL; + srs_freep(mock_controller); + srs_freep(mock_hub); +} + // Test SrsFragmentedMp4 delegation to fragment_ member // This test covers the major use scenario for SrsFragmentedMp4 ISrsFragment interface delegation VOID TEST(FragmentedMp4Test, FragmentDelegation) diff --git a/trunk/src/utest/srs_utest_ai17.hpp b/trunk/src/utest/srs_utest_ai17.hpp index 034ca36ce..dd30b839a 100644 --- a/trunk/src/utest/srs_utest_ai17.hpp +++ b/trunk/src/utest/srs_utest_ai17.hpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include diff --git a/trunk/src/utest/srs_utest_manual_mock.hpp b/trunk/src/utest/srs_utest_manual_mock.hpp index 0835e66b9..e8fce4aca 100644 --- a/trunk/src/utest/srs_utest_manual_mock.hpp +++ b/trunk/src/utest/srs_utest_manual_mock.hpp @@ -311,6 +311,7 @@ public: bool rtc_to_rtmp_; srs_utime_t dash_dispose_; bool dash_enabled_; + srs_utime_t hls_dispose_; bool api_as_candidates_; bool resolve_api_domain_; bool keep_api_domain_; @@ -343,6 +344,7 @@ public: rtc_to_rtmp_ = false; dash_dispose_ = 0; dash_enabled_ = false; + hls_dispose_ = 0; api_as_candidates_ = true; resolve_api_domain_ = true; keep_api_domain_ = false; @@ -562,7 +564,7 @@ public: virtual srs_utime_t get_hls_window(std::string vhost) { return 60 * SRS_UTIME_SECONDS; } virtual std::string get_hls_on_error(std::string vhost) { return "continue"; } virtual bool get_hls_cleanup(std::string vhost) { return true; } - virtual srs_utime_t get_hls_dispose(std::string vhost) { return 120 * SRS_UTIME_SECONDS; } + virtual srs_utime_t get_hls_dispose(std::string vhost) { return hls_dispose_; } virtual bool get_hls_wait_keyframe(std::string vhost) { return true; } virtual bool get_hls_keys(std::string vhost) { return false; } virtual int get_hls_fragments_per_key(std::string vhost) { return 5; }