From 85aa4b0746d62e05a02059e511693d4571993f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dar=C3=ADo=20Kondratiuk?= Date: Mon, 15 Jul 2024 17:50:28 -0300 Subject: [PATCH] Ensure temp directory is deleted on disposal (#2689) * Ensure temp directory is deleted on disposal * fix condition * Is this making all the noise? * Fix bad merge * Change locks * Fix build * Prettier * bump version * new approach * Change function name * small feedback * rollback to dispose * cr * Update lib/PuppeteerSharp/Browser.cs * undo refactors --- .../LauncherTests/BrowserCloseTests.cs | 46 +++++++++++-------- lib/PuppeteerSharp/Browser.cs | 9 ++-- lib/PuppeteerSharp/Helpers/TempDirectory.cs | 13 ++---- lib/PuppeteerSharp/LauncherBase.cs | 4 +- lib/PuppeteerSharp/PuppeteerSharp.csproj | 8 ++-- lib/PuppeteerSharp/States/DisposedState.cs | 24 ++++++---- lib/PuppeteerSharp/States/ExitedState.cs | 26 +++++++---- lib/PuppeteerSharp/States/ExitingState.cs | 4 +- lib/PuppeteerSharp/States/KillingState.cs | 12 ++--- .../States/ProcessStartingState.cs | 8 ++-- lib/PuppeteerSharp/States/StartedState.cs | 4 +- lib/PuppeteerSharp/States/State.cs | 4 +- 12 files changed, 88 insertions(+), 74 deletions(-) diff --git a/lib/PuppeteerSharp.Tests/LauncherTests/BrowserCloseTests.cs b/lib/PuppeteerSharp.Tests/LauncherTests/BrowserCloseTests.cs index bba2de405..75c444e70 100644 --- a/lib/PuppeteerSharp.Tests/LauncherTests/BrowserCloseTests.cs +++ b/lib/PuppeteerSharp.Tests/LauncherTests/BrowserCloseTests.cs @@ -2,34 +2,42 @@ using NUnit.Framework; using PuppeteerSharp.Nunit; -namespace PuppeteerSharp.Tests.BrowserTests.Events +namespace PuppeteerSharp.Tests.LauncherTests { public class BrowserCloseTests : PuppeteerBrowserBaseTest { - public BrowserCloseTests() : base() - { - } - [Test, Retry(2), PuppeteerTest("launcher.spec", "Launcher specs Browser.close", "should terminate network waiters")] public async Task ShouldTerminateNetworkWaiters() { - await using (var browser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions())) - await using (var remote = await Puppeteer.ConnectAsync(new ConnectOptions { BrowserWSEndpoint = browser.WebSocketEndpoint })) - { - var newPage = await remote.NewPageAsync(); - var requestTask = newPage.WaitForRequestAsync(TestConstants.EmptyPage); - var responseTask = newPage.WaitForResponseAsync(TestConstants.EmptyPage); + await using var browser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions()); + await using var remote = await Puppeteer.ConnectAsync(new ConnectOptions { BrowserWSEndpoint = browser.WebSocketEndpoint }); + var newPage = await remote.NewPageAsync(); + var requestTask = newPage.WaitForRequestAsync(TestConstants.EmptyPage); + var responseTask = newPage.WaitForResponseAsync(TestConstants.EmptyPage); + + await browser.CloseAsync(); - await browser.CloseAsync(); + var exception = Assert.ThrowsAsync(() => requestTask); + StringAssert.Contains("Target closed", exception.Message); + StringAssert.DoesNotContain("Timeout", exception.Message); + + exception = Assert.ThrowsAsync(() => responseTask); + StringAssert.Contains("Target closed", exception.Message); + StringAssert.DoesNotContain("Timeout", exception.Message); + } + + [Test] + public async Task DeleteTempUserDataDirWhenDisposingBrowser() + { + var options = TestConstants.DefaultBrowserOptions(); + var launcher = new Launcher(TestConstants.LoggerFactory); + await using var browser = await launcher.LaunchAsync(options); - var exception = Assert.ThrowsAsync(() => requestTask); - StringAssert.Contains("Target closed", exception.Message); - StringAssert.DoesNotContain("Timeout", exception.Message); + var tempUserDataDir = ((Browser)browser).Launcher.TempUserDataDir; + DirectoryAssert.Exists(tempUserDataDir.Path); - exception = Assert.ThrowsAsync(() => responseTask); - StringAssert.Contains("Target closed", exception.Message); - StringAssert.DoesNotContain("Timeout", exception.Message); - } + await browser.DisposeAsync(); + DirectoryAssert.DoesNotExist(tempUserDataDir.Path); } } } diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index 1af9241d7..d935494c0 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Threading.Tasks; using PuppeteerSharp.Cdp; -using PuppeteerSharp.Cdp.Messaging; using PuppeteerSharp.Helpers; namespace PuppeteerSharp @@ -59,13 +58,13 @@ public abstract class Browser : IBrowser internal TaskQueue ScreenshotTaskQueue { get; } = new(); - internal Connection Connection { get; set; } + internal Connection Connection { get; init; } - internal ViewPortOptions DefaultViewport { get; set; } + internal ViewPortOptions DefaultViewport { get; init; } - internal LauncherBase Launcher { get; set; } + internal LauncherBase Launcher { get; init; } - internal Func IsPageTargetFunc { get; set; } + internal Func IsPageTargetFunc { get; init; } /// public abstract Task NewPageAsync(); diff --git a/lib/PuppeteerSharp/Helpers/TempDirectory.cs b/lib/PuppeteerSharp/Helpers/TempDirectory.cs index ea108cb7e..8cd7d4470 100644 --- a/lib/PuppeteerSharp/Helpers/TempDirectory.cs +++ b/lib/PuppeteerSharp/Helpers/TempDirectory.cs @@ -44,22 +44,17 @@ public void Dispose() public override string ToString() => Path; - private static async Task DeleteAsync(string path) + public async Task DeleteAsync() { const int minDelayInMillis = 200; const int maxDelayInMillis = 8000; var retryDelay = minDelayInMillis; - while (true) + while (Directory.Exists(Path)) { - if (!Directory.Exists(path)) - { - return; - } - try { - Directory.Delete(path, true); + Directory.Delete(Path, true); return; } catch @@ -80,7 +75,7 @@ private void DisposeCore() return; } - _ = DeleteAsync(Path); + _ = DeleteAsync(); } } } diff --git a/lib/PuppeteerSharp/LauncherBase.cs b/lib/PuppeteerSharp/LauncherBase.cs index 66d1f1310..13f2bd11e 100644 --- a/lib/PuppeteerSharp/LauncherBase.cs +++ b/lib/PuppeteerSharp/LauncherBase.cs @@ -21,7 +21,7 @@ public abstract class LauncherBase : IDisposable /// /// Full path of executable. /// Options for launching Base. - public LauncherBase(string executable, LaunchOptions options) + protected LauncherBase(string executable, LaunchOptions options) { _stateManager = new StateManager(); _stateManager.Starting = new ProcessStartingState(_stateManager); @@ -80,7 +80,7 @@ public LauncherBase(string executable, LaunchOptions options) internal LaunchOptions Options { get; } - internal TempDirectory TempUserDataDir { get; set; } + internal TempDirectory TempUserDataDir { get; init; } /// /// Gets Base process current state. diff --git a/lib/PuppeteerSharp/PuppeteerSharp.csproj b/lib/PuppeteerSharp/PuppeteerSharp.csproj index 7899ff8e4..f91b92eca 100644 --- a/lib/PuppeteerSharp/PuppeteerSharp.csproj +++ b/lib/PuppeteerSharp/PuppeteerSharp.csproj @@ -12,10 +12,10 @@ Headless Browser .NET API PuppeteerSharp - 18.0.3 - 18.0.3 - 18.0.3 - 18.0.3 + 18.0.4 + 18.0.4 + 18.0.4 + 18.0.4 false false embedded diff --git a/lib/PuppeteerSharp/States/DisposedState.cs b/lib/PuppeteerSharp/States/DisposedState.cs index e44ed9505..cabe79775 100644 --- a/lib/PuppeteerSharp/States/DisposedState.cs +++ b/lib/PuppeteerSharp/States/DisposedState.cs @@ -3,23 +3,29 @@ namespace PuppeteerSharp.States { - internal class DisposedState : State + internal class DisposedState(StateManager stateManager) : State(stateManager) { - public DisposedState(StateManager stateManager) : base(stateManager) - { - } - - public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) + public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) { if (fromState == StateManager.Exited) { return Task.CompletedTask; } - Kill(p); + Kill(launcher); - p.ExitCompletionSource.TrySetException(new ObjectDisposedException(p.ToString())); - p.TempUserDataDir?.Dispose(); + if (launcher.TempUserDataDir is { } tempUserDataDir) + { + tempUserDataDir + .DeleteAsync() + .ContinueWith( + t => launcher.ExitCompletionSource.TrySetException(new ObjectDisposedException(launcher.ToString())), + TaskScheduler.Default); + } + else + { + launcher.ExitCompletionSource.TrySetException(new ObjectDisposedException(launcher.ToString())); + } return Task.CompletedTask; } diff --git a/lib/PuppeteerSharp/States/ExitedState.cs b/lib/PuppeteerSharp/States/ExitedState.cs index a03297381..76a78cf87 100644 --- a/lib/PuppeteerSharp/States/ExitedState.cs +++ b/lib/PuppeteerSharp/States/ExitedState.cs @@ -3,18 +3,14 @@ namespace PuppeteerSharp.States { - internal class ExitedState : State + internal class ExitedState(StateManager stateManager) : State(stateManager) { - public ExitedState(StateManager stateManager) : base(stateManager) + public void EnterFrom(LauncherBase launcher, State fromState) { - } - - public void EnterFrom(LauncherBase p, State fromState) - { - while (!StateManager.TryEnter(p, fromState, this)) + while (!StateManager.TryEnter(launcher, fromState, this)) { // Current state has changed since transition to this state was requested. - // Therefore retry transition to this state from the current state. This ensures + // Therefore, retry transition to this state from the current state. This ensures // that Leave() operation of current state is properly called. fromState = StateManager.CurrentState; if (fromState == this) @@ -23,8 +19,18 @@ public void EnterFrom(LauncherBase p, State fromState) } } - p.ExitCompletionSource.TrySetResult(true); - p.TempUserDataDir?.Dispose(); + if (launcher.TempUserDataDir is { } tempUserDataDir) + { + tempUserDataDir + .DeleteAsync() + .ContinueWith( + t => launcher.ExitCompletionSource.TrySetResult(true), + TaskScheduler.Default); + } + else + { + launcher.ExitCompletionSource.TrySetResult(true); + } } public override Task ExitAsync(LauncherBase p, TimeSpan timeout) => Task.CompletedTask; diff --git a/lib/PuppeteerSharp/States/ExitingState.cs b/lib/PuppeteerSharp/States/ExitingState.cs index 1686afba1..8d9613476 100644 --- a/lib/PuppeteerSharp/States/ExitingState.cs +++ b/lib/PuppeteerSharp/States/ExitingState.cs @@ -11,8 +11,8 @@ public ExitingState(StateManager stateManager) : base(stateManager) { } - public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) - => !StateManager.TryEnter(p, fromState, this) ? StateManager.CurrentState.ExitAsync(p, timeout) : ExitAsync(p, timeout); + public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) + => !StateManager.TryEnter(launcher, fromState, this) ? StateManager.CurrentState.ExitAsync(launcher, timeout) : ExitAsync(launcher, timeout); public override async Task ExitAsync(LauncherBase p, TimeSpan timeout) { diff --git a/lib/PuppeteerSharp/States/KillingState.cs b/lib/PuppeteerSharp/States/KillingState.cs index 59d068ae4..f5fea6b71 100644 --- a/lib/PuppeteerSharp/States/KillingState.cs +++ b/lib/PuppeteerSharp/States/KillingState.cs @@ -9,20 +9,20 @@ public KillingState(StateManager stateManager) : base(stateManager) { } - public override async Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) + public override async Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) { - if (!StateManager.TryEnter(p, fromState, this)) + if (!StateManager.TryEnter(launcher, fromState, this)) { // Delegate KillAsync to current state, because it has already changed since // transition to this state was initiated. - await StateManager.CurrentState.KillAsync(p).ConfigureAwait(false); + await StateManager.CurrentState.KillAsync(launcher).ConfigureAwait(false); } try { - if (!p.Process.HasExited) + if (!launcher.Process.HasExited) { - p.Process.Kill(); + launcher.Process.Kill(); } } catch (InvalidOperationException) @@ -31,7 +31,7 @@ public override async Task EnterFromAsync(LauncherBase p, State fromState, TimeS return; } - await WaitForExitAsync(p).ConfigureAwait(false); + await WaitForExitAsync(launcher).ConfigureAwait(false); } public override Task ExitAsync(LauncherBase p, TimeSpan timeout) => WaitForExitAsync(p); diff --git a/lib/PuppeteerSharp/States/ProcessStartingState.cs b/lib/PuppeteerSharp/States/ProcessStartingState.cs index d00bf1c9c..6b9ed179d 100644 --- a/lib/PuppeteerSharp/States/ProcessStartingState.cs +++ b/lib/PuppeteerSharp/States/ProcessStartingState.cs @@ -13,16 +13,16 @@ public ProcessStartingState(StateManager stateManager) : base(stateManager) { } - public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) + public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) { - if (!StateManager.TryEnter(p, fromState, this)) + if (!StateManager.TryEnter(launcher, fromState, this)) { // Delegate StartAsync to current state, because it has already changed since // transition to this state was initiated. - return StateManager.CurrentState.StartAsync(p); + return StateManager.CurrentState.StartAsync(launcher); } - return StartCoreAsync(p); + return StartCoreAsync(launcher); } public override Task StartAsync(LauncherBase p) => p.StartCompletionSource.Task; diff --git a/lib/PuppeteerSharp/States/StartedState.cs b/lib/PuppeteerSharp/States/StartedState.cs index 62800212e..6c65d901d 100644 --- a/lib/PuppeteerSharp/States/StartedState.cs +++ b/lib/PuppeteerSharp/States/StartedState.cs @@ -9,9 +9,9 @@ public StartedState(StateManager stateManager) : base(stateManager) { } - public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) + public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) { - StateManager.TryEnter(p, fromState, this); + StateManager.TryEnter(launcher, fromState, this); return Task.CompletedTask; } diff --git a/lib/PuppeteerSharp/States/State.cs b/lib/PuppeteerSharp/States/State.cs index eee427560..b7ae7f90e 100644 --- a/lib/PuppeteerSharp/States/State.cs +++ b/lib/PuppeteerSharp/States/State.cs @@ -65,9 +65,9 @@ public State(StateManager stateManager) public bool IsExited => this == StateManager.Exited || this == StateManager.Disposed; - public virtual Task EnterFromAsync(LauncherBase p, State fromState) => EnterFromAsync(p, fromState, TimeSpan.Zero); + public virtual Task EnterFromAsync(LauncherBase launcher, State fromState) => EnterFromAsync(launcher, fromState, TimeSpan.Zero); - public virtual Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout) => Task.FromException(InvalidOperation("enterFrom")); + public virtual Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout) => Task.FromException(InvalidOperation("enterFrom")); public virtual Task StartAsync(LauncherBase p) => Task.FromException(InvalidOperation("start"));