Skip to content

Commit 85aa4b0

Browse files
kblokkblok
authored
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
1 parent 7d7e6f0 commit 85aa4b0

File tree

12 files changed

+88
-74
lines changed

12 files changed

+88
-74
lines changed

‎lib/PuppeteerSharp.Tests/LauncherTests/BrowserCloseTests.cs‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,34 +2,42 @@
22
using NUnit.Framework;
33
using PuppeteerSharp.Nunit;
44

5-
namespace PuppeteerSharp.Tests.BrowserTests.Events
5+
namespace PuppeteerSharp.Tests.LauncherTests
66
{
77
public class BrowserCloseTests : PuppeteerBrowserBaseTest
88
{
9-
public BrowserCloseTests() : base()
10-
{
11-
}
12-
139
[Test, Retry(2), PuppeteerTest("launcher.spec", "Launcher specs Browser.close", "should terminate network waiters")]
1410
public async Task ShouldTerminateNetworkWaiters()
1511
{
16-
await using (var browser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions()))
17-
await using (var remote = await Puppeteer.ConnectAsync(new ConnectOptions { BrowserWSEndpoint = browser.WebSocketEndpoint }))
18-
{
19-
var newPage = await remote.NewPageAsync();
20-
var requestTask = newPage.WaitForRequestAsync(TestConstants.EmptyPage);
21-
var responseTask = newPage.WaitForResponseAsync(TestConstants.EmptyPage);
12+
await using var browser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions());
13+
await using var remote = await Puppeteer.ConnectAsync(new ConnectOptions { BrowserWSEndpoint = browser.WebSocketEndpoint });
14+
var newPage = await remote.NewPageAsync();
15+
var requestTask = newPage.WaitForRequestAsync(TestConstants.EmptyPage);
16+
var responseTask = newPage.WaitForResponseAsync(TestConstants.EmptyPage);
17+
18+
await browser.CloseAsync();
2219

23-
await browser.CloseAsync();
20+
var exception = Assert.ThrowsAsync<TargetClosedException>(() => requestTask);
21+
StringAssert.Contains("Target closed", exception.Message);
22+
StringAssert.DoesNotContain("Timeout", exception.Message);
23+
24+
exception = Assert.ThrowsAsync<TargetClosedException>(() => responseTask);
25+
StringAssert.Contains("Target closed", exception.Message);
26+
StringAssert.DoesNotContain("Timeout", exception.Message);
27+
}
28+
29+
[Test]
30+
public async Task DeleteTempUserDataDirWhenDisposingBrowser()
31+
{
32+
var options = TestConstants.DefaultBrowserOptions();
33+
var launcher = new Launcher(TestConstants.LoggerFactory);
34+
await using var browser = await launcher.LaunchAsync(options);
2435

25-
var exception = Assert.ThrowsAsync<TargetClosedException>(() => requestTask);
26-
StringAssert.Contains("Target closed", exception.Message);
27-
StringAssert.DoesNotContain("Timeout", exception.Message);
36+
var tempUserDataDir = ((Browser)browser).Launcher.TempUserDataDir;
37+
DirectoryAssert.Exists(tempUserDataDir.Path);
2838

29-
exception = Assert.ThrowsAsync<TargetClosedException>(() => responseTask);
30-
StringAssert.Contains("Target closed", exception.Message);
31-
StringAssert.DoesNotContain("Timeout", exception.Message);
32-
}
39+
await browser.DisposeAsync();
40+
DirectoryAssert.DoesNotExist(tempUserDataDir.Path);
3341
}
3442
}
3543
}

‎lib/PuppeteerSharp/Browser.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using System.Linq;
55
using System.Threading.Tasks;
66
using PuppeteerSharp.Cdp;
7-
using PuppeteerSharp.Cdp.Messaging;
87
using PuppeteerSharp.Helpers;
98

109
namespace PuppeteerSharp
@@ -59,13 +58,13 @@ public abstract class Browser : IBrowser
5958

6059
internal TaskQueue ScreenshotTaskQueue { get; } = new();
6160

62-
internal Connection Connection { get; set; }
61+
internal Connection Connection { get; init; }
6362

64-
internal ViewPortOptions DefaultViewport { get; set; }
63+
internal ViewPortOptions DefaultViewport { get; init; }
6564

66-
internal LauncherBase Launcher { get; set; }
65+
internal LauncherBase Launcher { get; init; }
6766

68-
internal Func<Target, bool> IsPageTargetFunc { get; set; }
67+
internal Func<Target, bool> IsPageTargetFunc { get; init; }
6968

7069
/// <inheritdoc/>
7170
public abstract Task<IPage> NewPageAsync();

‎lib/PuppeteerSharp/Helpers/TempDirectory.cs‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,22 +44,17 @@ public void Dispose()
4444

4545
public override string ToString() => Path;
4646

47-
private static async Task DeleteAsync(string path)
47+
public async Task DeleteAsync()
4848
{
4949
const int minDelayInMillis = 200;
5050
const int maxDelayInMillis = 8000;
5151

5252
var retryDelay = minDelayInMillis;
53-
while (true)
53+
while (Directory.Exists(Path))
5454
{
55-
if (!Directory.Exists(path))
56-
{
57-
return;
58-
}
59-
6055
try
6156
{
62-
Directory.Delete(path, true);
57+
Directory.Delete(Path, true);
6358
return;
6459
}
6560
catch
@@ -80,7 +75,7 @@ private void DisposeCore()
8075
return;
8176
}
8277

83-
_ = DeleteAsync(Path);
78+
_ = DeleteAsync();
8479
}
8580
}
8681
}

‎lib/PuppeteerSharp/LauncherBase.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public abstract class LauncherBase : IDisposable
2121
/// </summary>
2222
/// <param name="executable">Full path of executable.</param>
2323
/// <param name="options">Options for launching Base.</param>
24-
public LauncherBase(string executable, LaunchOptions options)
24+
protected LauncherBase(string executable, LaunchOptions options)
2525
{
2626
_stateManager = new StateManager();
2727
_stateManager.Starting = new ProcessStartingState(_stateManager);
@@ -80,7 +80,7 @@ public LauncherBase(string executable, LaunchOptions options)
8080

8181
internal LaunchOptions Options { get; }
8282

83-
internal TempDirectory TempUserDataDir { get; set; }
83+
internal TempDirectory TempUserDataDir { get; init; }
8484

8585
/// <summary>
8686
/// Gets Base process current state.

‎lib/PuppeteerSharp/PuppeteerSharp.csproj‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
<Description>Headless Browser .NET API</Description>
1313
<PackageId>PuppeteerSharp</PackageId>
1414
<PackageReleaseNotes></PackageReleaseNotes>
15-
<PackageVersion>18.0.3</PackageVersion>
16-
<ReleaseVersion>18.0.3</ReleaseVersion>
17-
<AssemblyVersion>18.0.3</AssemblyVersion>
18-
<FileVersion>18.0.3</FileVersion>
15+
<PackageVersion>18.0.4</PackageVersion>
16+
<ReleaseVersion>18.0.4</ReleaseVersion>
17+
<AssemblyVersion>18.0.4</AssemblyVersion>
18+
<FileVersion>18.0.4</FileVersion>
1919
<SynchReleaseVersion>false</SynchReleaseVersion>
2020
<StyleCopTreatErrorsAsWarnings>false</StyleCopTreatErrorsAsWarnings>
2121
<DebugType>embedded</DebugType>

‎lib/PuppeteerSharp/States/DisposedState.cs‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,29 @@
33

44
namespace PuppeteerSharp.States
55
{
6-
internal class DisposedState : State
6+
internal class DisposedState(StateManager stateManager) : State(stateManager)
77
{
8-
public DisposedState(StateManager stateManager) : base(stateManager)
9-
{
10-
}
11-
12-
public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout)
8+
public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout)
139
{
1410
if (fromState == StateManager.Exited)
1511
{
1612
return Task.CompletedTask;
1713
}
1814

19-
Kill(p);
15+
Kill(launcher);
2016

21-
p.ExitCompletionSource.TrySetException(new ObjectDisposedException(p.ToString()));
22-
p.TempUserDataDir?.Dispose();
17+
if (launcher.TempUserDataDir is { } tempUserDataDir)
18+
{
19+
tempUserDataDir
20+
.DeleteAsync()
21+
.ContinueWith(
22+
t => launcher.ExitCompletionSource.TrySetException(new ObjectDisposedException(launcher.ToString())),
23+
TaskScheduler.Default);
24+
}
25+
else
26+
{
27+
launcher.ExitCompletionSource.TrySetException(new ObjectDisposedException(launcher.ToString()));
28+
}
2329

2430
return Task.CompletedTask;
2531
}

‎lib/PuppeteerSharp/States/ExitedState.cs‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,14 @@
33

44
namespace PuppeteerSharp.States
55
{
6-
internal class ExitedState : State
6+
internal class ExitedState(StateManager stateManager) : State(stateManager)
77
{
8-
public ExitedState(StateManager stateManager) : base(stateManager)
8+
public void EnterFrom(LauncherBase launcher, State fromState)
99
{
10-
}
11-
12-
public void EnterFrom(LauncherBase p, State fromState)
13-
{
14-
while (!StateManager.TryEnter(p, fromState, this))
10+
while (!StateManager.TryEnter(launcher, fromState, this))
1511
{
1612
// Current state has changed since transition to this state was requested.
17-
// Therefore retry transition to this state from the current state. This ensures
13+
// Therefore, retry transition to this state from the current state. This ensures
1814
// that Leave() operation of current state is properly called.
1915
fromState = StateManager.CurrentState;
2016
if (fromState == this)
@@ -23,8 +19,18 @@ public void EnterFrom(LauncherBase p, State fromState)
2319
}
2420
}
2521

26-
p.ExitCompletionSource.TrySetResult(true);
27-
p.TempUserDataDir?.Dispose();
22+
if (launcher.TempUserDataDir is { } tempUserDataDir)
23+
{
24+
tempUserDataDir
25+
.DeleteAsync()
26+
.ContinueWith(
27+
t => launcher.ExitCompletionSource.TrySetResult(true),
28+
TaskScheduler.Default);
29+
}
30+
else
31+
{
32+
launcher.ExitCompletionSource.TrySetResult(true);
33+
}
2834
}
2935

3036
public override Task ExitAsync(LauncherBase p, TimeSpan timeout) => Task.CompletedTask;

‎lib/PuppeteerSharp/States/ExitingState.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ public ExitingState(StateManager stateManager) : base(stateManager)
1111
{
1212
}
1313

14-
public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout)
15-
=> !StateManager.TryEnter(p, fromState, this) ? StateManager.CurrentState.ExitAsync(p, timeout) : ExitAsync(p, timeout);
14+
public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout)
15+
=> !StateManager.TryEnter(launcher, fromState, this) ? StateManager.CurrentState.ExitAsync(launcher, timeout) : ExitAsync(launcher, timeout);
1616

1717
public override async Task ExitAsync(LauncherBase p, TimeSpan timeout)
1818
{

‎lib/PuppeteerSharp/States/KillingState.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,20 @@ public KillingState(StateManager stateManager) : base(stateManager)
99
{
1010
}
1111

12-
public override async Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout)
12+
public override async Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout)
1313
{
14-
if (!StateManager.TryEnter(p, fromState, this))
14+
if (!StateManager.TryEnter(launcher, fromState, this))
1515
{
1616
// Delegate KillAsync to current state, because it has already changed since
1717
// transition to this state was initiated.
18-
await StateManager.CurrentState.KillAsync(p).ConfigureAwait(false);
18+
await StateManager.CurrentState.KillAsync(launcher).ConfigureAwait(false);
1919
}
2020

2121
try
2222
{
23-
if (!p.Process.HasExited)
23+
if (!launcher.Process.HasExited)
2424
{
25-
p.Process.Kill();
25+
launcher.Process.Kill();
2626
}
2727
}
2828
catch (InvalidOperationException)
@@ -31,7 +31,7 @@ public override async Task EnterFromAsync(LauncherBase p, State fromState, TimeS
3131
return;
3232
}
3333

34-
await WaitForExitAsync(p).ConfigureAwait(false);
34+
await WaitForExitAsync(launcher).ConfigureAwait(false);
3535
}
3636

3737
public override Task ExitAsync(LauncherBase p, TimeSpan timeout) => WaitForExitAsync(p);

‎lib/PuppeteerSharp/States/ProcessStartingState.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,16 @@ public ProcessStartingState(StateManager stateManager) : base(stateManager)
1313
{
1414
}
1515

16-
public override Task EnterFromAsync(LauncherBase p, State fromState, TimeSpan timeout)
16+
public override Task EnterFromAsync(LauncherBase launcher, State fromState, TimeSpan timeout)
1717
{
18-
if (!StateManager.TryEnter(p, fromState, this))
18+
if (!StateManager.TryEnter(launcher, fromState, this))
1919
{
2020
// Delegate StartAsync to current state, because it has already changed since
2121
// transition to this state was initiated.
22-
return StateManager.CurrentState.StartAsync(p);
22+
return StateManager.CurrentState.StartAsync(launcher);
2323
}
2424

25-
return StartCoreAsync(p);
25+
return StartCoreAsync(launcher);
2626
}
2727

2828
public override Task StartAsync(LauncherBase p) => p.StartCompletionSource.Task;

0 commit comments

Comments
 (0)