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
hardkoded · Jul 15, 2024 · 85aa4b0 · 85aa4b0
1 parent 7d7e6f0
commit 85aa4b0
Show file tree

Hide file tree

Showing 12 changed files with 88 additions and 74 deletions.
diff --git 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<TargetClosedException>(() => requestTask);
+            StringAssert.Contains("Target closed", exception.Message);
+            StringAssert.DoesNotContain("Timeout", exception.Message);
+
+            exception = Assert.ThrowsAsync<TargetClosedException>(() => 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<TargetClosedException>(() => 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<TargetClosedException>(() => 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
@@ -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<Target, bool> IsPageTargetFunc { get; set; }
+        internal Func<Target, bool> IsPageTargetFunc { get; init; }
 
         /// <inheritdoc/>
         public abstract Task<IPage> NewPageAsync();

diff --git 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
@@ -21,7 +21,7 @@ public abstract class LauncherBase : IDisposable
         /// </summary>
         /// <param name="executable">Full path of executable.</param>
         /// <param name="options">Options for launching Base.</param>
-        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; }
 
         /// <summary>
         /// Gets Base process current state.

diff --git a/lib/PuppeteerSharp/PuppeteerSharp.csproj b/lib/PuppeteerSharp/PuppeteerSharp.csproj
@@ -12,10 +12,10 @@
     <Description>Headless Browser .NET API</Description>
     <PackageId>PuppeteerSharp</PackageId>
     <PackageReleaseNotes></PackageReleaseNotes>
-    <PackageVersion>18.0.3</PackageVersion>
-    <ReleaseVersion>18.0.3</ReleaseVersion>
-    <AssemblyVersion>18.0.3</AssemblyVersion>
-    <FileVersion>18.0.3</FileVersion>
+    <PackageVersion>18.0.4</PackageVersion>
+    <ReleaseVersion>18.0.4</ReleaseVersion>
+    <AssemblyVersion>18.0.4</AssemblyVersion>
+    <FileVersion>18.0.4</FileVersion>
     <SynchReleaseVersion>false</SynchReleaseVersion>
     <StyleCopTreatErrorsAsWarnings>false</StyleCopTreatErrorsAsWarnings>
     <DebugType>embedded</DebugType>

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"));