New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #543: robust temporary user directory cleanup #544

Merged
merged 17 commits into from Sep 1, 2018

Conversation

Projects
None yet
2 participants
@ggeurts
Contributor

ggeurts commented Aug 24, 2018

First commit contains changes to Launcher class to Ensure robust cleanup of temporary user directories and avoid exceptions in Process event handlers.

Additional commits solve various failing tests on Windows.

@@ -112,7 +112,7 @@ public async Task ShouldWorkWithComplicatedUsecases()
await Page.GoToAsync(TestConstants.ServerUrl + "/csscoverage/involved.html");
var coverage = await Page.Coverage.StopCSSCoverageAsync();
Assert.Equal(
TestUtils.CompressText(involved),
TestUtils.CompressText(JsonConvert.SerializeObject(JsonConvert.DeserializeObject(involved))),

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Why do we need this?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Test failed because of difference in line breaks (\r characters).

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

I'd change CompressText to remove \n and \r instead.

@@ -1,31 +1,31 @@
using System;

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Are you changing anything in this class?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

No, I suspect this was due to VS2017 making line breaks consistent

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Let's revert the change then.

@@ -5,6 +5,9 @@
namespace PuppeteerSharp.Tests
{
using Newtonsoft.Json;
using Xunit;

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Do we need this?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

No, they will be removed in a clean-up of the commits for the unit tests

protected virtual async Task DisposeAsync() => await Browser.CloseAsync();
public void Dispose() => DisposeAsync().GetAwaiter().GetResult();
protected virtual async Task DisposeAsync()
=> await Browser.CloseAsync();

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

This can fit in one line

@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netcoreapp2.0;net471</TargetFrameworks>
<TargetFrameworks>net471;netcoreapp2.0</TargetFrameworks>

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Any reason for this change?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Nope, will adjust

Dispose(true);
}
protected async void Dispose(bool disposing)

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

I don't know if you can async a Dispose. Do you have any doc for that?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Async void methods are fire-and-forget async operations. They are not recommended unless you really know what you are doing. In this case the method does a best attempt to delete the directory, which may take some time, which justifies spawning an asynchronous operation for which we don't want to wait on the outcome.

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

We are implementing fire-and-forget in this way

_ = Delete();

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

What is the policy for dealing with exceptions in fire-and-forget methods? If the Delete() operation throws an exception it can kill the process when the Task object gets finalized.

This comment has been minimized.

@kblok

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Thanks, I wasn't yet aware of their existence and behavior when used with Tasks.

@@ -13,6 +13,8 @@
namespace PuppeteerSharp
{
using System.Text;

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Let's move the using with the rest

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Agree

@@ -168,17 +172,16 @@ public async Task<Browser> ConnectAsync(ConnectOptions options)
var response = await _connection.SendAsync<GetBrowserContextsResponse>("Target.getBrowserContexts");
return await Browser.CreateAsync(_connection, response.BrowserContextIds, options.IgnoreHTTPSErrors, true, null, () =>
return await Browser.CreateAsync(_connection, response.BrowserContextIds, options.IgnoreHTTPSErrors, true, null, async () =>

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

We need this not to be async and returning a task instead because the Dispose is not async and we can't await there
https://github.com/kblok/puppeteer-sharp/blob/master/lib/PuppeteerSharp/Browser.cs#L249

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

I am not sure I understand this comment. The Browser class takes a Func<Task> argument, which is what is created here. The async () => … syntax is needed to allow use of await and enable logging of close exceptions.

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Here I'm speaking more from previous experiences and pain :p
Browser.CloseAsync is quite tricky. It behaves nicely when you call it explicitly. But it gave us some headaches when it's being called from the Dispose, because of the GetAwaiter().GetResult() so we need there to get a task. If we can we will await it, if we can't we will fire-and-forget.

You can leave it and try but your test on the CI will get stuck.

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

I'd go for the fire-and-forget then in the Dispose().

{
_waitForChromeToClose.SetResult(true);
cts?.Dispose();

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Why do you need to dispose the token?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Because it is disposable. It may well hold onto a timer itself which shouldn't be kept alive if it is no longer needed.

}
}
catch (InvalidOperationException ex) when (ex.Message == "No process is associated with this object.")
catch (InvalidOperationException)

This comment has been minimized.

@kblok

kblok Aug 24, 2018

Owner

Why are you changing this?

This comment has been minimized.

@ggeurts

ggeurts Aug 24, 2018

Contributor

Checking for an english message is a recipe for failure when running in other locales

@ggeurts ggeurts force-pushed the ggeurts:temp-user-data-cleanup branch from e2fcb16 to 8614232 Aug 24, 2018

@kblok

This comment has been minimized.

Owner

kblok commented Aug 24, 2018

BTW. Thanks for your contribution @ggeurts and taking the time to contribute. I appreciate that a lot.

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Aug 27, 2018

@kblok Ready making changes and processing reviews. A lot of the launcher code has ended up in new ChromeProcess class, which will do its best to clean up chrome processes and temporary user data directories.

Show resolved Hide resolved lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs Outdated
@@ -2,6 +2,8 @@
namespace PuppeteerSharp.Tests
{
using System.Text;

This comment has been minimized.

@kblok

kblok Aug 27, 2018

Owner

usings should be outside the namespace.

Show resolved Hide resolved lib/PuppeteerSharp.Tests/TestUtils.cs
/// Represents a Chrome process and any associated temporary user data directory that have created
/// by Puppeteer and therefore must be cleaned up when no longer needed.
/// </summary>
public class ChromeProcess : IDisposable

This comment has been minimized.

@kblok

kblok Aug 27, 2018

Owner

I'd call this ChromiumProcess

This comment has been minimized.

@ggeurts

ggeurts Aug 28, 2018

Contributor

And rename ChromeProcessException to ChromiumProcessException?

#region Private methods
private static List<string> PrepareChromeArgs(LaunchOptions options, out TempDirectory tempUserDataDir)

This comment has been minimized.

@kblok

kblok Aug 27, 2018

Owner

We could return a tuple instead of using out parameters

Show resolved Hide resolved lib/PuppeteerSharp/Helpers/TempDirectory.cs
/// <summary>
/// Finalizer.
/// </summary>
~ChromeProcess()

This comment has been minimized.

@kblok

kblok Aug 27, 2018

Owner

Do we need to implement finalizers?

@kblok

almost there almost there

Show resolved Hide resolved lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs Outdated

@kblok kblok requested a review from Meir017 Aug 28, 2018

@kblok

This comment has been minimized.

Owner

kblok commented Aug 28, 2018

@Meir017 do you want to step in?

@kblok kblok merged commit 3d311be into kblok:master Sep 1, 2018

2 checks passed

CodeFactor 1 issue fixed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kblok

This comment has been minimized.

Owner

kblok commented Sep 1, 2018

Thanks a lot @ggeurts!

kblok added a commit that referenced this pull request Sep 1, 2018

@kblok

This comment has been minimized.

Owner

kblok commented Sep 1, 2018

I had to revert to commit @ggeurts. I had many process leaks on MacOS. Feel free to open another PR.

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Sep 2, 2018

Hmm, my wife considers this a valid reason to purchase a Mac... Will try to find a way to run tests on MacOS.

@kblok

This comment has been minimized.

Owner

kblok commented Sep 3, 2018

If you add this

LoggerFactory.AddDebug((log, LogLevel) => log.Contains("Launcher"));

here https://github.com/kblok/puppeteer-sharp/blob/master/lib/PuppeteerSharp.Tests/TestConstants.cs#L59

And you run tests in debug mode, you could also see if you are getting leaks on Windows.

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Sep 3, 2018

I can reproduce process leaks on Windows when XUnit logging is enabled. The XUnit logger throws an exception on writing a log message when the test is no longer active.

@kblok

This comment has been minimized.

Owner

kblok commented Sep 3, 2018

That's good news!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment