From b984da547576bf5851eb85730a4c18d836cc6f11 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Tue, 4 Sep 2018 10:04:37 +0200 Subject: [PATCH 1/9] Make string compacting independent of Windows/Unix line end styles --- lib/PuppeteerSharp.Tests/TestUtils.cs | 41 ++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/PuppeteerSharp.Tests/TestUtils.cs b/lib/PuppeteerSharp.Tests/TestUtils.cs index 463fd83b0..e25322b21 100644 --- a/lib/PuppeteerSharp.Tests/TestUtils.cs +++ b/lib/PuppeteerSharp.Tests/TestUtils.cs @@ -1,4 +1,5 @@ using System.IO; +using System.Text; namespace PuppeteerSharp.Tests { @@ -14,6 +15,44 @@ public static string FindParentDirectory(string directory) return Path.Combine(current, directory); } - public static string CompressText(string text) => text.Replace("\n", "").Replace("\t", "").Replace(" ", ""); + /// + /// Removes as much whitespace as possible from a given string. Whitespace + /// that separates letters and/or digits is collapsed to a space character. + /// Other whitespace is fully removed. + /// + public static string CompressText(string text) + { + if (string.IsNullOrEmpty(text)) + { + return text; + } + + var sb = new StringBuilder(); + var inWhitespace = false; + foreach (var ch in text) + { + if (char.IsWhiteSpace(ch)) + { + if (ch != '\n' && ch != '\r') + { + inWhitespace = true; + } + } + else + { + if (inWhitespace) + { + inWhitespace = false; + if (sb.Length > 0 && char.IsLetterOrDigit(sb[sb.Length - 1]) && char.IsLetterOrDigit(ch)) + { + sb.Append(' '); + } + } + sb.Append(ch); + } + } + + return sb.ToString(); + } } } From 9a1cd1820c62c35197ccec643a285f31a935ee1f Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Tue, 4 Sep 2018 10:06:50 +0200 Subject: [PATCH 2/9] Avoid Async-to-Sync transition in tests --- .../PuppeteerBrowserBaseTest.cs | 15 +++++---------- lib/PuppeteerSharp.Tests/PuppeteerPageBaseTest.cs | 4 ++-- .../TracingTests/TracingTests.cs | 2 +- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs b/lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs index cbe91b223..bd3ac72e5 100644 --- a/lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs +++ b/lib/PuppeteerSharp.Tests/PuppeteerBrowserBaseTest.cs @@ -1,11 +1,11 @@ -using System; -using System.IO; +using System.IO; using System.Threading.Tasks; +using Xunit; using Xunit.Abstractions; namespace PuppeteerSharp.Tests { - public class PuppeteerBrowserBaseTest : PuppeteerBaseTest, IDisposable + public class PuppeteerBrowserBaseTest : PuppeteerBaseTest, IAsyncLifetime { protected Browser Browser { get; set; } @@ -18,16 +18,11 @@ public PuppeteerBrowserBaseTest(ITestOutputHelper output) : base(output) { dirInfo.Create(); } - - InitializeAsync().GetAwaiter().GetResult(); } - protected virtual async Task InitializeAsync() - { + public virtual async Task InitializeAsync() => Browser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions(), TestConstants.LoggerFactory); - } - protected virtual async Task DisposeAsync() => await Browser.CloseAsync(); - public void Dispose() => DisposeAsync().GetAwaiter().GetResult(); + public virtual async Task DisposeAsync() => await Browser.CloseAsync(); } } \ No newline at end of file diff --git a/lib/PuppeteerSharp.Tests/PuppeteerPageBaseTest.cs b/lib/PuppeteerSharp.Tests/PuppeteerPageBaseTest.cs index 18424f32f..27ff6e4f8 100644 --- a/lib/PuppeteerSharp.Tests/PuppeteerPageBaseTest.cs +++ b/lib/PuppeteerSharp.Tests/PuppeteerPageBaseTest.cs @@ -11,13 +11,13 @@ public PuppeteerPageBaseTest(ITestOutputHelper output) : base(output) protected Page Page { get; set; } - protected override async Task InitializeAsync() + public override async Task InitializeAsync() { await base.InitializeAsync(); Page = await Browser.NewPageAsync(); } - protected override async Task DisposeAsync() + public override async Task DisposeAsync() { await Page.CloseAsync(); await base.DisposeAsync(); diff --git a/lib/PuppeteerSharp.Tests/TracingTests/TracingTests.cs b/lib/PuppeteerSharp.Tests/TracingTests/TracingTests.cs index 56cdad35d..7d988db68 100644 --- a/lib/PuppeteerSharp.Tests/TracingTests/TracingTests.cs +++ b/lib/PuppeteerSharp.Tests/TracingTests/TracingTests.cs @@ -21,7 +21,7 @@ public TracingTests(ITestOutputHelper output) : base(output) _file = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); } - protected override async Task DisposeAsync() + public override async Task DisposeAsync() { await base.DisposeAsync(); From 6b6e803c7ccf26e2a0402abd12797f3074455615 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Tue, 4 Sep 2018 10:08:56 +0200 Subject: [PATCH 3/9] Ensure unit test runs consistently on Windows and non-Windows platforms --- lib/PuppeteerSharp.Tests/FrameTests/FrameManagementTests.cs | 4 +++- .../PuppeteerTests/PuppeteerConnectTests.cs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/PuppeteerSharp.Tests/FrameTests/FrameManagementTests.cs b/lib/PuppeteerSharp.Tests/FrameTests/FrameManagementTests.cs index ac05bbdea..d722cd579 100644 --- a/lib/PuppeteerSharp.Tests/FrameTests/FrameManagementTests.cs +++ b/lib/PuppeteerSharp.Tests/FrameTests/FrameManagementTests.cs @@ -17,7 +17,9 @@ public FrameManagementTests(ITestOutputHelper output) : base(output) public async Task ShouldHandleNestedFrames() { await Page.GoToAsync(TestConstants.ServerUrl + "/frames/nested-frames.html"); - Assert.Equal(TestConstants.NestedFramesDumpResult, FrameUtils.DumpFrames(Page.MainFrame)); + Assert.Equal( + TestUtils.CompressText(TestConstants.NestedFramesDumpResult), + TestUtils.CompressText(FrameUtils.DumpFrames(Page.MainFrame))); } [Fact] diff --git a/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerConnectTests.cs b/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerConnectTests.cs index 7f7642f41..1d57b27b7 100644 --- a/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerConnectTests.cs +++ b/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerConnectTests.cs @@ -71,7 +71,7 @@ public async Task ShouldBeAbleToReconnectToADisconnectedBrowser() var restoredPage = pages.FirstOrDefault(x => x.Url == url); Assert.NotNull(restoredPage); var frameDump = FrameUtils.DumpFrames(restoredPage.MainFrame); - Assert.Equal(TestConstants.NestedFramesDumpResult, frameDump); + Assert.Equal(TestUtils.CompressText(TestConstants.NestedFramesDumpResult), TestUtils.CompressText(frameDump)); var response = await restoredPage.EvaluateExpressionAsync("7 * 8"); Assert.Equal(56, response); } From 0a61436dad09b28d69404112e7a0c60a637a8247 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Tue, 4 Sep 2018 10:09:27 +0200 Subject: [PATCH 4/9] Prevent test failure due to undeleted directory of previous test run --- .../PuppeteerTests/BrowserFetcherTests.cs | 53 ++++++++++++------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/lib/PuppeteerSharp.Tests/PuppeteerTests/BrowserFetcherTests.cs b/lib/PuppeteerSharp.Tests/PuppeteerTests/BrowserFetcherTests.cs index 0f3b4bc39..4f9986122 100644 --- a/lib/PuppeteerSharp.Tests/PuppeteerTests/BrowserFetcherTests.cs +++ b/lib/PuppeteerSharp.Tests/PuppeteerTests/BrowserFetcherTests.cs @@ -9,16 +9,21 @@ namespace PuppeteerSharp.Tests.PuppeteerTests [Collection("PuppeteerLoaderFixture collection")] public class BrowserFetcherTests : PuppeteerBaseTest { - public BrowserFetcherTests(ITestOutputHelper output) : base(output) { } + private readonly string _downloadsFolder; + + public BrowserFetcherTests(ITestOutputHelper output) : base(output) + { + _downloadsFolder = Path.Combine(Directory.GetCurrentDirectory(), ".test-chromium"); + EnsureDownloadsFolderIsDeleted(); + } [Fact] public async Task ShouldDownloadAndExtractLinuxBinary() { - var downloadsFolder = Path.Combine(Directory.GetCurrentDirectory(), ".test-chromium"); var browserFetcher = Puppeteer.CreateBrowserFetcher(new BrowserFetcherOptions { Platform = Platform.Linux, - Path = downloadsFolder, + Path = _downloadsFolder, Host = TestConstants.ServerUrl }); var revisionInfo = browserFetcher.RevisionInfo(123456); @@ -29,22 +34,34 @@ public async Task ShouldDownloadAndExtractLinuxBinary() Assert.False(await browserFetcher.CanDownloadAsync(100000)); Assert.True(await browserFetcher.CanDownloadAsync(123456)); - revisionInfo = await browserFetcher.DownloadAsync(123456); - Assert.True(revisionInfo.Local); - Assert.Equal("LINUX BINARY\n", File.ReadAllText(revisionInfo.ExecutablePath)); - Assert.Equal(new[] { 123456 }, browserFetcher.LocalRevisions()); - browserFetcher.Remove(123456); - Assert.Empty(browserFetcher.LocalRevisions()); - - //Download should return data from a downloaded version - //This section is not in the Puppeteer test. - await browserFetcher.DownloadAsync(123456); - Server.Reset(); - revisionInfo = await browserFetcher.DownloadAsync(123456); - Assert.True(revisionInfo.Local); - Assert.Equal("LINUX BINARY\n", File.ReadAllText(revisionInfo.ExecutablePath)); + try + { + revisionInfo = await browserFetcher.DownloadAsync(123456); + Assert.True(revisionInfo.Local); + Assert.Equal("LINUX BINARY\n", File.ReadAllText(revisionInfo.ExecutablePath)); + Assert.Equal(new[] {123456}, browserFetcher.LocalRevisions()); + browserFetcher.Remove(123456); + Assert.Empty(browserFetcher.LocalRevisions()); - new DirectoryInfo(downloadsFolder).Delete(true); + //Download should return data from a downloaded version + //This section is not in the Puppeteer test. + await browserFetcher.DownloadAsync(123456); + Server.Reset(); + revisionInfo = await browserFetcher.DownloadAsync(123456); + Assert.True(revisionInfo.Local); + Assert.Equal("LINUX BINARY\n", File.ReadAllText(revisionInfo.ExecutablePath)); + } + finally + { + EnsureDownloadsFolderIsDeleted(); + } + } + private void EnsureDownloadsFolderIsDeleted() + { + if (Directory.Exists(_downloadsFolder)) + { + Directory.Delete(_downloadsFolder, true); + } } } } \ No newline at end of file From b30afbbeafcfd1622e393cdc445a55af837e02c9 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Tue, 4 Sep 2018 10:10:34 +0200 Subject: [PATCH 5/9] Prevent leaks of chromium processes and temp user directories --- lib/PuppeteerSharp.Tests/Issues/Issue0128.cs | 2 +- .../PuppeteerTests/PuppeteerLaunchTests.cs | 162 ++-- lib/PuppeteerSharp.sln | 5 +- lib/PuppeteerSharp/Browser.cs | 77 +- lib/PuppeteerSharp/ChromeProcessException.cs | 27 - lib/PuppeteerSharp/ChromiumProcess.cs | 721 ++++++++++++++++++ .../ChromiumProcessException.cs | 27 + lib/PuppeteerSharp/Connection.cs | 2 +- lib/PuppeteerSharp/Helpers/TempDirectory.cs | 87 +++ lib/PuppeteerSharp/Launcher.cs | 473 ++---------- lib/PuppeteerSharp/Puppeteer.cs | 2 +- 11 files changed, 1037 insertions(+), 548 deletions(-) delete mode 100644 lib/PuppeteerSharp/ChromeProcessException.cs create mode 100644 lib/PuppeteerSharp/ChromiumProcess.cs create mode 100644 lib/PuppeteerSharp/ChromiumProcessException.cs create mode 100644 lib/PuppeteerSharp/Helpers/TempDirectory.cs diff --git a/lib/PuppeteerSharp.Tests/Issues/Issue0128.cs b/lib/PuppeteerSharp.Tests/Issues/Issue0128.cs index 8c693e280..3ef7cb752 100644 --- a/lib/PuppeteerSharp.Tests/Issues/Issue0128.cs +++ b/lib/PuppeteerSharp.Tests/Issues/Issue0128.cs @@ -11,7 +11,7 @@ public class Issue0128 [Fact] public async Task LauncherShouldFailGracefully() { - await Assert.ThrowsAsync(async () => + await Assert.ThrowsAsync(async () => { var options = TestConstants.DefaultBrowserOptions(); options.Args = new[] { "-remote-debugging-port=-2" }; diff --git a/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerLaunchTests.cs b/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerLaunchTests.cs index 71c4eaf45..2df3c4d80 100644 --- a/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerLaunchTests.cs +++ b/lib/PuppeteerSharp.Tests/PuppeteerTests/PuppeteerLaunchTests.cs @@ -6,6 +6,7 @@ using System.Net; using System.Runtime.InteropServices; using System.Threading.Tasks; +using PuppeteerSharp.Helpers; using Xunit; using Xunit.Abstractions; @@ -119,116 +120,118 @@ public async Task ShouldRejectIfExecutablePathIsInvalid() [Fact] public async Task UserDataDirOption() { - var launcher = new Launcher(TestConstants.LoggerFactory); - var userDataDir = Launcher.GetTemporaryDirectory(); - var options = TestConstants.DefaultBrowserOptions(); - options.UserDataDir = userDataDir; - - using (var browser = await launcher.LaunchAsync(options)) + using (var userDataDir = new TempDirectory()) { - Assert.True(Directory.GetFiles(userDataDir).Length > 0); - await browser.CloseAsync(); - Assert.True(Directory.GetFiles(userDataDir).Length > 0); - await launcher.TryDeleteUserDataDir(); + var options = TestConstants.DefaultBrowserOptions(); + options.UserDataDir = userDataDir.Path; + + var launcher = new Launcher(TestConstants.LoggerFactory); + using (var browser = await launcher.LaunchAsync(options)) + { + Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0); + await browser.CloseAsync(); + Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0); + } } } [Fact] public async Task UserDataDirArgument() { - var launcher = new Launcher(TestConstants.LoggerFactory); - var userDataDir = Launcher.GetTemporaryDirectory(); - var options = TestConstants.DefaultBrowserOptions(); - options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray(); - - using (var browser = await launcher.LaunchAsync(options)) + using (var userDataDir = new TempDirectory()) { - // Open a page to make sure its functional. - await browser.NewPageAsync(); - Assert.True(Directory.GetFiles(userDataDir).Length > 0); - await browser.CloseAsync(); - Assert.True(Directory.GetFiles(userDataDir).Length > 0); - await launcher.TryDeleteUserDataDir(); + var launcher = new Launcher(TestConstants.LoggerFactory); + var options = TestConstants.DefaultBrowserOptions(); + options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray(); + + using (var browser = await launcher.LaunchAsync(options)) + { + // Open a page to make sure its functional. + await browser.NewPageAsync(); + Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0); + await browser.CloseAsync(); + Assert.True(Directory.GetFiles(userDataDir.Path).Length > 0); + } } } [Fact] public async Task UserDataDirOptionShouldRestoreState() { - var launcher = new Launcher(TestConstants.LoggerFactory); - var userDataDir = Launcher.GetTemporaryDirectory(); - var options = TestConstants.DefaultBrowserOptions(); - options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray(); - - using (var browser = await launcher.LaunchAsync(options)) + using (var userDataDir = new TempDirectory()) { - var page = await browser.NewPageAsync(); - await page.GoToAsync(TestConstants.EmptyPage); - await page.EvaluateExpressionAsync("localStorage.hey = 'hello'"); - } + var launcher = new Launcher(TestConstants.LoggerFactory); + var options = TestConstants.DefaultBrowserOptions(); + options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray(); - using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) - { - var page2 = await browser2.NewPageAsync(); - await page2.GoToAsync(TestConstants.EmptyPage); - Assert.Equal("hello", await page2.EvaluateExpressionAsync("localStorage.hey")); - } + using (var browser = await launcher.LaunchAsync(options)) + { + var page = await browser.NewPageAsync(); + await page.GoToAsync(TestConstants.EmptyPage); + await page.EvaluateExpressionAsync("localStorage.hey = 'hello'"); + } - await launcher.TryDeleteUserDataDir(); + using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) + { + var page2 = await browser2.NewPageAsync(); + await page2.GoToAsync(TestConstants.EmptyPage); + Assert.Equal("hello", await page2.EvaluateExpressionAsync("localStorage.hey")); + } + } } [Fact] public async Task UserDataDirOptionShouldRestoreCookies() { - var launcher = new Launcher(TestConstants.LoggerFactory); - var userDataDir = Launcher.GetTemporaryDirectory(); - var options = TestConstants.DefaultBrowserOptions(); - options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray(); - - using (var browser = await launcher.LaunchAsync(options)) + using (var userDataDir = new TempDirectory()) { - var page = await browser.NewPageAsync(); - await page.GoToAsync(TestConstants.EmptyPage); - await page.EvaluateExpressionAsync( - "document.cookie = 'doSomethingOnlyOnce=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'"); - } + var launcher = new Launcher(TestConstants.LoggerFactory); + var options = TestConstants.DefaultBrowserOptions(); + options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray(); - using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) - { - var page2 = await browser2.NewPageAsync(); - await page2.GoToAsync(TestConstants.EmptyPage); - Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync("document.cookie")); - } + using (var browser = await launcher.LaunchAsync(options)) + { + var page = await browser.NewPageAsync(); + await page.GoToAsync(TestConstants.EmptyPage); + await page.EvaluateExpressionAsync( + "document.cookie = 'doSomethingOnlyOnce=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'"); + } - await launcher.TryDeleteUserDataDir(); + using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) + { + var page2 = await browser2.NewPageAsync(); + await page2.GoToAsync(TestConstants.EmptyPage); + Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync("document.cookie")); + } + } } [Fact] public async Task HeadlessShouldBeAbleToReadCookiesWrittenByHeadful() { - var launcher = new Launcher(TestConstants.LoggerFactory); - var userDataDir = Launcher.GetTemporaryDirectory(); - var options = TestConstants.DefaultBrowserOptions(); - options.Args = options.Args.Concat(new[] { $"--user-data-dir=\"{userDataDir}\"" }).ToArray(); - options.Headless = false; - - using (var browser = await launcher.LaunchAsync(options)) + using (var userDataDir = new TempDirectory()) { - var page = await browser.NewPageAsync(); - await page.GoToAsync(TestConstants.EmptyPage); - await page.EvaluateExpressionAsync( - "document.cookie = 'foo=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'"); - } + var launcher = new Launcher(TestConstants.LoggerFactory); + var options = TestConstants.DefaultBrowserOptions(); + options.Args = options.Args.Concat(new[] {$"--user-data-dir=\"{userDataDir}\""}).ToArray(); + options.Headless = false; - options.Headless = true; - using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) - { - var page2 = await browser2.NewPageAsync(); - await page2.GoToAsync(TestConstants.EmptyPage); - Assert.Equal("foo=true", await page2.EvaluateExpressionAsync("document.cookie")); - } + using (var browser = await launcher.LaunchAsync(options)) + { + var page = await browser.NewPageAsync(); + await page.GoToAsync(TestConstants.EmptyPage); + await page.EvaluateExpressionAsync( + "document.cookie = 'foo=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'"); + } - await launcher.TryDeleteUserDataDir(); + options.Headless = true; + using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory)) + { + var page2 = await browser2.NewPageAsync(); + await page2.GoToAsync(TestConstants.EmptyPage); + Assert.Equal("foo=true", await page2.EvaluateExpressionAsync("document.cookie")); + } + } } [Fact] @@ -278,7 +281,7 @@ public async Task ChromeShouldBeClosed() await browser.CloseAsync(); - Assert.True(launcher.IsChromeClosed); + Assert.True(launcher.Process.HasExited); } } @@ -295,7 +298,8 @@ public async Task ChromeShouldBeClosedOnDispose() Assert.Equal(HttpStatusCode.OK, response.Status); } - Assert.True(launcher.IsChromeClosed); + Assert.True(await launcher.Process.WaitForExitAsync(TimeSpan.FromSeconds(10))); + Assert.True(launcher.Process.HasExited); } [Fact] diff --git a/lib/PuppeteerSharp.sln b/lib/PuppeteerSharp.sln index 37505239a..84b92a185 100644 --- a/lib/PuppeteerSharp.sln +++ b/lib/PuppeteerSharp.sln @@ -10,7 +10,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.TestServer", EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.Tests.DumpIO", "PuppeteerSharp.Tests.DumpIO\PuppeteerSharp.Tests.DumpIO.csproj", "{B1892212-CEE3-4DC3-ADB8-04A2D9A081A0}" EndProject -Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PuppeteerSharp.Tests.CloseMe", "PuppeteerSharp.Tests.CloseMe\PuppeteerSharp.Tests.CloseMe.csproj", "{B1B0358F-88A2-4038-9974-7850D906C76B}" +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "PuppeteerSharp.Tests.CloseMe", "PuppeteerSharp.Tests.CloseMe\PuppeteerSharp.Tests.CloseMe.csproj", "{B1B0358F-88A2-4038-9974-7850D906C76B}" EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution @@ -45,6 +45,9 @@ Global GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {F9F7978B-89C0-4A54-8D80-A1E93E401D4C} EndGlobalSection + GlobalSection(ExtensibilityGlobals) = postSolution + SolutionGuid = {F9F7978B-89C0-4A54-8D80-A1E93E401D4C} + EndGlobalSection GlobalSection(MonoDevelopProperties) = preSolution Policies = $0 $0.DotNetNamingPolicy = $1 diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index 12d7e056a..9420b9ebb 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -42,17 +42,14 @@ public class Browser : IDisposable /// The context ids> /// The option to ignoreHTTPSErrors /// The option to setDefaultViewport - /// The chrome process - /// An async function called before closing + /// The Chromium process public Browser( Connection connection, string[] contextIds, bool ignoreHTTPSErrors, bool setDefaultViewport, - Process process, - Func closeCallBack) + ChromiumProcess chromiumProcess) { - Process = process; Connection = connection; IgnoreHTTPSErrors = ignoreHTTPSErrors; SetDefaultViewport = setDefaultViewport; @@ -65,7 +62,7 @@ public Browser( Connection.Closed += (object sender, EventArgs e) => Disconnected?.Invoke(this, new EventArgs()); Connection.MessageReceived += Connect_MessageReceived; - _closeCallBack = closeCallBack; + _chromiumProcess = chromiumProcess; _logger = Connection.LoggerFactory.CreateLogger(); } @@ -74,9 +71,11 @@ public Browser( internal readonly Dictionary TargetsMap; private readonly Dictionary _contexts; - private readonly Func _closeCallBack; private readonly ILogger _logger; private readonly BrowserContext _defaultContext; + private readonly ChromiumProcess _chromiumProcess; + private Task _closeTask; + #endregion #region Properties @@ -123,7 +122,7 @@ public Browser( /// /// Gets the spawned browser process. Returns null if the browser instance was created with method. /// - public Process Process { get; } + public Process Process => _chromiumProcess?.Process; /// /// Gets or Sets whether to ignore HTTPS errors during navigation @@ -133,7 +132,7 @@ public Browser( /// /// Gets a value indicating if the browser is closed /// - public bool IsClosed { get; internal set; } + public bool IsClosed => _closeTask != null && _closeTask.IsCompleted && _closeTask.Exception != null; internal TaskQueue ScreenshotTaskQueue { get; set; } internal Connection Connection { get; } @@ -236,24 +235,46 @@ public async Task GetUserAgentAsync() /// Closes Chromium and all of its pages (if any were opened). The browser object itself is considered disposed and cannot be used anymore /// /// Task - public async Task CloseAsync() + public Task CloseAsync() => _closeTask ?? (_closeTask = CloseCoreAsync()); + + private async Task CloseCoreAsync() { - if (IsClosed) + Connection.StopReading(); + try { - return; + try + { + // Initiate graceful browser close operation but don't await it just yet, + // because we want to ensure chromium process shutdown first. + var browserCloseTask = Connection.SendAsync("Browser.close", null); + + if (_chromiumProcess != null) + { + // Notify chromium process that exit is expected, but should be enforced if it + // doesn't occur withing the close timeout. + var closeTimeout = TimeSpan.FromMilliseconds(5000); + await _chromiumProcess.EnsureExitAsync(closeTimeout).ConfigureAwait(false); + } + + // Now we can safely await the browser close operation without risking keeping chromium + // process running for indeterminate period. + await browserCloseTask.ConfigureAwait(false); + } + finally + { + Disconnect(); + } } - - IsClosed = true; - Connection.StopReading(); - - var closeTask = _closeCallBack(); - - if (closeTask != null) + catch (Exception ex) { - await closeTask.ConfigureAwait(false); + _logger.LogError(ex, ex.Message); + + if (_chromiumProcess != null) + { + await _chromiumProcess.KillAsync().ConfigureAwait(false); + } } - Disconnect(); Closed?.Invoke(this, new EventArgs()); } @@ -372,10 +393,9 @@ internal static async Task CreateAsync( string[] contextIds, bool ignoreHTTPSErrors, bool appMode, - Process process, - Func closeCallBack) + ChromiumProcess chromiumProcess) { - var browser = new Browser(connection, contextIds, ignoreHTTPSErrors, appMode, process, closeCallBack); + var browser = new Browser(connection, contextIds, ignoreHTTPSErrors, appMode, chromiumProcess); await connection.SendAsync("Target.setDiscoverTargets", new { discover = true @@ -386,8 +406,13 @@ internal static async Task CreateAsync( #endregion #region IDisposable - /// - public void Dispose() => CloseAsync().GetAwaiter().GetResult(); + + /// + /// Closes and any Chromium that was + /// created by Puppeteer. + /// + public void Dispose() => _ = CloseAsync(); + #endregion } } \ No newline at end of file diff --git a/lib/PuppeteerSharp/ChromeProcessException.cs b/lib/PuppeteerSharp/ChromeProcessException.cs deleted file mode 100644 index 4220c95a1..000000000 --- a/lib/PuppeteerSharp/ChromeProcessException.cs +++ /dev/null @@ -1,27 +0,0 @@ -using System; - -namespace PuppeteerSharp -{ - /// - /// Chrome process exception thrown by . - /// - public class ChromeProcessException : PuppeteerException - { - /// - /// Initializes a new instance of the class. - /// - /// Message. - public ChromeProcessException(string message) : base(message) - { - } - - /// - /// Initializes a new instance of the class. - /// - /// Message. - /// Inner exception. - public ChromeProcessException(string message, Exception innerException) : base(message, innerException) - { - } - } -} \ No newline at end of file diff --git a/lib/PuppeteerSharp/ChromiumProcess.cs b/lib/PuppeteerSharp/ChromiumProcess.cs new file mode 100644 index 000000000..d15c866ad --- /dev/null +++ b/lib/PuppeteerSharp/ChromiumProcess.cs @@ -0,0 +1,721 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Text; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using PuppeteerSharp.Helpers; + +namespace PuppeteerSharp +{ + /// + /// Represents a Chromium process and any associated temporary user data directory that have created + /// by Puppeteer and therefore must be cleaned up when no longer needed. + /// + public class ChromiumProcess : IDisposable + { + #region Constants + + internal static readonly string[] DefaultArgs = { + "--disable-background-networking", + "--disable-background-timer-throttling", + "--disable-breakpad", + "--disable-client-side-phishing-detection", + "--disable-default-apps", + "--disable-dev-shm-usage", + "--disable-extensions", + "--disable-features=site-per-process", + "--disable-hang-monitor", + "--disable-popup-blocking", + "--disable-prompt-on-repost", + "--disable-sync", + "--disable-translate", + "--metrics-recording-only", + "--no-first-run", + "--safebrowsing-disable-auto-update" + }; + + internal static readonly string[] AutomationArgs = { + "--enable-automation", + "--password-store=basic", + "--use-mock-keychain" + }; + + private const string UserDataDirArgument = "--user-data-dir"; + + #endregion + + #region Static fields + + private static int _processCount; + + #endregion + + #region Instance fields + + private readonly LaunchOptions _options; + private readonly TempDirectory _tempUserDataDir; + private readonly ILogger _logger; + private readonly TaskCompletionSource _startCompletionSource = new TaskCompletionSource(); + private readonly TaskCompletionSource _exitCompletionSource = new TaskCompletionSource(); + private State _currentState = State.Initial; + + #endregion + + #region Constructor + + /// + /// Creates a new instance. + /// + /// Full path of Chromium executable. + /// Options for launching Chromium. + /// Logger factory + public ChromiumProcess(string chromiumExecutable, LaunchOptions options, ILoggerFactory loggerFactory) + { + _options = options; + _logger = options.LogProcess + ? loggerFactory.CreateLogger() + : null; + + List chromiumArgs; + (chromiumArgs, _tempUserDataDir) = PrepareChromiumArgs(options); + + Process = new Process + { + EnableRaisingEvents = true + }; + Process.StartInfo.UseShellExecute = false; + Process.StartInfo.FileName = chromiumExecutable; + Process.StartInfo.Arguments = string.Join(" ", chromiumArgs); + Process.StartInfo.RedirectStandardError = true; + + SetEnvVariables(Process.StartInfo.Environment, options.Env, Environment.GetEnvironmentVariables()); + + if (options.DumpIO) + { + Process.ErrorDataReceived += (sender, e) => Console.Error.WriteLine(e.Data); + } + } + + #endregion + + #region Dispose + + /// + /// Finalizer. + /// + ~ChromiumProcess() + { + Dispose(false); + } + + /// + public void Dispose() + { + GC.SuppressFinalize(this); + Dispose(true); + } + + /// + /// Disposes Chromium process and any temporary user directory. + /// + /// Indicates whether disposal was initiated by operation. + protected virtual void Dispose(bool disposing) => _currentState.Dispose(this); + + #endregion + + #region Properties + + /// + /// Gets Chromium process details. + /// + public Process Process { get; } + + /// + /// Gets Chromium endpoint. + /// + public string EndPoint => _startCompletionSource.Task.IsCompleted + ? _startCompletionSource.Task.Result + : null; + + /// + /// Indicates whether Chromium process is exiting. + /// + public bool IsExiting => _currentState.IsExiting; + + /// + /// Indicates whether Chromium process has exited. + /// + public bool HasExited => _currentState.IsExited; + + #endregion + + #region Public methods + + /// + /// Asynchronously starts Chromium process. + /// + /// + public Task StartAsync() => _currentState.StartAsync(this); + + /// + /// Asynchronously waits for graceful Chromium process exit within a given timeout period. + /// Kills the Chromium process if it has not exited within this period. + /// + /// The maximum waiting time for a graceful process exit. + /// + public Task EnsureExitAsync(TimeSpan? timeout) => timeout.HasValue + ? _currentState.ExitAsync(this, timeout.Value) + : _currentState.KillAsync(this); + + /// + /// Asynchronously kills Chromium process. + /// + /// + public Task KillAsync() => _currentState.KillAsync(this); + + /// + /// Waits for Chromium process exit within a given timeout. + /// + /// The maximum wait period. + /// true if Chromium process has exited within the given , + /// or false otherwise. + public async Task WaitForExitAsync(TimeSpan? timeout) + { + if (timeout.HasValue) + { + var completedTask = await Task.WhenAny(_exitCompletionSource.Task, Task.Delay(timeout.Value)).ConfigureAwait(false); + return completedTask == _exitCompletionSource.Task; + } + + await _exitCompletionSource.Task.ConfigureAwait(false); + return true; + } + + /// + public override string ToString() => $"Chromium process; EndPoint={EndPoint}; State={_currentState}"; + + #endregion + + #region Private methods + + private static (List chromiumArgs, TempDirectory tempUserDataDir) PrepareChromiumArgs(LaunchOptions options) + { + var chromiumArgs = new List(DefaultArgs); + TempDirectory tempUserDataDir = null; + + if (options.AppMode) + { + options.Headless = false; + } + else + { + chromiumArgs.AddRange(AutomationArgs); + } + + if (!options.IgnoreDefaultArgs || + !chromiumArgs.Any(argument => argument.StartsWith("--remote-debugging-", StringComparison.Ordinal))) + { + chromiumArgs.Add("--remote-debugging-port=0"); + } + + var userDataDirOption = options.Args.FirstOrDefault(i => i.StartsWith(UserDataDirArgument, StringComparison.Ordinal)); + if (string.IsNullOrEmpty(userDataDirOption)) + { + if (string.IsNullOrEmpty(options.UserDataDir)) + { + tempUserDataDir = new TempDirectory(); + chromiumArgs.Add($"{UserDataDirArgument}={tempUserDataDir.Path.Quote()}"); + } + else + { + chromiumArgs.Add($"{UserDataDirArgument}={options.UserDataDir.Quote()}"); + } + } + else + { + options.UserDataDir = userDataDirOption.Replace($"{UserDataDirArgument}=", string.Empty).UnQuote(); + } + + if (options.Devtools) + { + chromiumArgs.Add("--auto-open-devtools-for-tabs"); + options.Headless = false; + } + + if (options.Headless) + { + chromiumArgs.AddRange(new[]{ + "--headless", + "--disable-gpu", + "--hide-scrollbars", + "--mute-audio" + }); + } + + if (!options.IgnoreDefaultArgs && options.Args.Any() && options.Args.All(arg => arg.StartsWith("-"))) + { + chromiumArgs.Add("about:blank"); + } + + if (options.Args.Any()) + { + chromiumArgs.AddRange(options.Args); + } + + return (chromiumArgs, tempUserDataDir); + } + + private static void SetEnvVariables(IDictionary environment, IDictionary customEnv, IDictionary realEnv) + { + foreach (DictionaryEntry item in realEnv) + { + environment[item.Key.ToString()] = item.Value.ToString(); + } + + if (customEnv != null) + { + foreach (var item in customEnv) + { + environment[item.Key] = item.Value; + } + } + } + + #endregion + + #region State machine + + /// + /// Represents state machine for Chromium process instances. The happy path runs along the + /// following state transitions: + /// -> + /// -> + /// -> + /// -> . + /// -> . + /// + /// + /// + /// This state machine implements the following state transitions: + /// + /// State Event Target State Action + /// ======== =================== ============ ========================================================== + /// Initial --StartAsync------> Starting Start process and wait for endpoint + /// Initial --ExitAsync-------> Exited Cleanup temp user data + /// Initial --KillAsync-------> Exited Cleanup temp user data + /// Initial --Dispose---------> Disposed Cleanup temp user data + /// Starting --StartAsync------> Starting - + /// Starting --ExitAsync-------> Exiting Wait for process exit + /// Starting --KillAsync-------> Killing Kill process + /// Starting --Dispose---------> Disposed Kill process; Cleanup temp user data; throw ObjectDisposedException on outstanding async operations; + /// Starting --endpoint ready--> Started Complete StartAsync successfully; Log process start + /// Starting --process exit----> Exited Complete StartAsync with exception; Cleanup temp user data + /// Started --StartAsync------> Started - + /// Started --EnsureExitAsync-> Exiting Start exit timer; Log process exit + /// Started --KillAsync-------> Killing Kill process; Log process exit + /// Started --Dispose---------> Disposed Kill process; Log process exit; Cleanup temp user data; throw ObjectDisposedException on outstanding async operations; + /// Started --process exit----> Exited Log process exit; Cleanup temp user data + /// Exiting --StartAsync------> Exiting - (StartAsync throws InvalidOperationException) + /// Exiting --ExitAsync-------> Exiting - + /// Exiting --KillAsync-------> Killing Kill process + /// Exiting --Dispose---------> Disposed Kill process; Cleanup temp user data; throw ObjectDisposedException on outstanding async operations; + /// Exiting --exit timeout----> Killing Kill process + /// Exiting --process exit----> Exited Cleanup temp user data; complete outstanding async operations; + /// Killing --StartAsync------> Killing - (StartAsync throws InvalidOperationException) + /// Killing --KillAsync-------> Killing - + /// Killing --Dispose---------> Disposed Cleanup temp user data; throw ObjectDisposedException on outstanding async operations; + /// Killing --process exit----> Exited Cleanup temp user data; complete outstanding async operations; + /// Exited --StartAsync------> Killing - (StartAsync throws InvalidOperationException) + /// Exited --KillAsync-------> Exited - + /// Exited --Dispose---------> Disposed - + /// Disposed --StartAsync------> Disposed - + /// Disposed --KillAsync-------> Disposed - + /// Disposed --Dispose---------> Disposed - + /// + /// + /// + private abstract class State + { + #region Predefined states + + public static readonly State Initial = new InitialState(); + private static readonly StartingState Starting = new StartingState(); + private static readonly StartedState Started = new StartedState(); + private static readonly ExitingState Exiting = new ExitingState(); + private static readonly KillingState Killing = new KillingState(); + private static readonly ExitedState Exited = new ExitedState(); + private static readonly DisposedState Disposed = new DisposedState(); + + #endregion + + #region Properties + + public bool IsExiting => this == Killing || this == Exiting; + public bool IsExited => this == Exited || this == Disposed; + + #endregion + + #region Methods + + /// + /// Attempts thread-safe transitions from a given state to this state. + /// + /// The Chromium process + /// The state from which state transition takes place + /// Returns true if transition is successful, or false if transition + /// cannot be made because current state does not equal . + protected bool TryEnter(ChromiumProcess p, State fromState) + { + if (Interlocked.CompareExchange(ref p._currentState, this, fromState) == fromState) + { + fromState.Leave(p); + return true; + } + + return false; + } + + /// + /// Notifies that state machine is about to transition to another state. + /// + /// The Chromium process + protected virtual void Leave(ChromiumProcess p) + { } + + /// + /// Handles process start request. + /// + /// The Chromium process + /// + public virtual Task StartAsync(ChromiumProcess p) => Task.FromException(InvalidOperation("start")); + + /// + /// Handles process exit request. + /// + /// The Chromium process + /// The maximum waiting time for a graceful process exit. + /// + public virtual Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => Task.FromException(InvalidOperation("exit")); + + /// + /// Handles process kill request. + /// + /// The Chromium process + /// + public virtual Task KillAsync(ChromiumProcess p) => Task.FromException(InvalidOperation("kill")); + + /// + /// Handles wait for process exit request. + /// + /// The Chromium process + /// + public virtual Task WaitForExitAsync(ChromiumProcess p) => p._exitCompletionSource.Task; + + /// + /// Handles disposal of process and temporary user directory + /// + /// + public virtual void Dispose(ChromiumProcess p) => Disposed.EnterFrom(p, this); + + public override string ToString() + { + var name = GetType().Name; + return name.Substring(0, name.Length - "State".Length); + } + + private Exception InvalidOperation(string operationName) + => new InvalidOperationException($"Cannot {operationName} in state {this}"); + + /// + /// Kills process if it is still alive. + /// + /// + private static void Kill(ChromiumProcess p) + { + try + { + if (!p.Process.HasExited) + { + p.Process.Kill(); + } + } + catch (InvalidOperationException) + { + // Ignore + } + } + + #endregion + + #region Concrete state classes + + private class InitialState : State + { + public override Task StartAsync(ChromiumProcess p) => Starting.EnterFromAsync(p, this); + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) + { + Exited.EnterFrom(p, this); + return Task.CompletedTask; + } + + public override Task KillAsync(ChromiumProcess p) + { + Exited.EnterFrom(p, this); + return Task.CompletedTask; + } + + public override Task WaitForExitAsync(ChromiumProcess p) => Task.FromException(InvalidOperation("wait for exit")); + } + + private class StartingState : State + { + public Task EnterFromAsync(ChromiumProcess p, State fromState) + { + if (!TryEnter(p, fromState)) + { + // Delegate StartAsync to current state, because it has already changed since + // transition to this state was initiated. + return p._currentState.StartAsync(p); + } + + return StartCoreAsync(p); + } + + public override Task StartAsync(ChromiumProcess p) => p._startCompletionSource.Task; + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => Exiting.EnterFromAsync(p, this, timeout); + + public override Task KillAsync(ChromiumProcess p) => Killing.EnterFromAsync(p, this); + + public override void Dispose(ChromiumProcess p) + { + p._startCompletionSource.TrySetException(new ObjectDisposedException(p.ToString())); + base.Dispose(p); + } + + private async Task StartCoreAsync(ChromiumProcess p) + { + var output = new StringBuilder(); + + void OnProcessDataReceivedWhileStarting(object sender, DataReceivedEventArgs e) + { + if (e.Data != null) + { + output.AppendLine(e.Data); + var match = Regex.Match(e.Data, "^DevTools listening on (ws:\\/\\/.*)"); + if (match.Success) + { + p._startCompletionSource.SetResult(match.Groups[1].Value); + } + } + } + void OnProcessExitedWhileStarting(object sender, EventArgs e) + { + p._startCompletionSource.SetException(new ChromiumProcessException($"Failed to launch Chromium! {output}")); + } + void OnProcessExited(object sender, EventArgs e) + { + Exited.EnterFrom(p, p._currentState); + } + + p.Process.ErrorDataReceived += OnProcessDataReceivedWhileStarting; + p.Process.Exited += OnProcessExitedWhileStarting; + p.Process.Exited += OnProcessExited; + CancellationTokenSource cts = null; + try + { + p.Process.Start(); + await Started.EnterFromAsync(p, this).ConfigureAwait(false); + + p.Process.BeginErrorReadLine(); + + var timeout = p._options.Timeout; + if (timeout > 0) + { + cts = new CancellationTokenSource(timeout); + cts.Token.Register(() => p._startCompletionSource.TrySetException( + new ChromiumProcessException($"Timed out after {timeout} ms while trying to connect to Chromium!"))); + } + + try + { + await p._startCompletionSource.Task.ConfigureAwait(false); + await Started.EnterFromAsync(p, this); + } + catch + { + await Killing.EnterFromAsync(p, this).ConfigureAwait(false); + throw; + } + } + finally + { + cts?.Dispose(); + p.Process.Exited -= OnProcessExitedWhileStarting; + p.Process.ErrorDataReceived -= OnProcessDataReceivedWhileStarting; + } + } + } + + private class StartedState : State + { + public Task EnterFromAsync(ChromiumProcess p, State fromState) + { + if (TryEnter(p, fromState)) + { + // Process has not exited or been killed since transition to this state was initiated + LogProcessCount(p, Interlocked.Increment(ref _processCount)); + } + return Task.CompletedTask; + } + + protected override void Leave(ChromiumProcess p) + => LogProcessCount(p, Interlocked.Decrement(ref _processCount)); + + public override Task StartAsync(ChromiumProcess p) => Task.CompletedTask; + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => Exiting.EnterFromAsync(p, this, timeout); + + public override Task KillAsync(ChromiumProcess p) => Killing.EnterFromAsync(p, this); + + private static void LogProcessCount(ChromiumProcess p, int processCount) + { + try + { + p._logger?.LogInformation("Process Count: {ProcessCount}", processCount); + } + catch + { + // Prevent logging exception from causing havoc + } + } + } + + private class ExitingState : State + { + public Task EnterFromAsync(ChromiumProcess p, State fromState, TimeSpan timeout) + { + if (!TryEnter(p, fromState)) + { + return p._currentState.ExitAsync(p, timeout); + } + + return ExitAsync(p, timeout); + } + + public override async Task ExitAsync(ChromiumProcess p, TimeSpan timeout) + { + var timeoutTask = Task.Delay(timeout); + var waitForExitTask = WaitForExitAsync(p); + var completedTask = await Task.WhenAny(waitForExitTask, timeoutTask).ConfigureAwait(false); + if (completedTask == timeoutTask) + { + await Killing.EnterFromAsync(p, this).ConfigureAwait(false); + await waitForExitTask.ConfigureAwait(false); + } + } + + public override Task KillAsync(ChromiumProcess p) => Killing.EnterFromAsync(p, this); + } + + private class KillingState : State + { + public Task EnterFromAsync(ChromiumProcess p, State fromState) + { + if (!TryEnter(p, fromState)) + { + // Delegate KillAsync to current state, because it has already changed since + // transition to this state was initiated. + return p._currentState.KillAsync(p); + } + + try + { + if (!p.Process.HasExited) + { + p.Process.Kill(); + } + } + catch (InvalidOperationException) + { + // Ignore + } + + return WaitForExitAsync(p); + } + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => WaitForExitAsync(p); + + public override Task KillAsync(ChromiumProcess p) => WaitForExitAsync(p); + } + + private class ExitedState : State + { + public void EnterFrom(ChromiumProcess p, State fromState) + { + while (!TryEnter(p, fromState)) + { + // Current state has changed since transition to this state was requested. + // Therefore retry transition to this state from the current state. This ensures + // that Leave() operation of current state is properly called. + fromState = p._currentState; + if (fromState == this) + { + return; + } + } + + p._exitCompletionSource.TrySetResult(true); + p._tempUserDataDir?.Dispose(); + } + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => Task.CompletedTask; + + public override Task KillAsync(ChromiumProcess p) => Task.CompletedTask; + + public override Task WaitForExitAsync(ChromiumProcess p) => Task.CompletedTask; + } + + private class DisposedState : State + { + public void EnterFrom(ChromiumProcess p, State fromState) + { + if (!TryEnter(p, fromState)) + { + // Delegate Dispose to current state, because it has already changed since + // transition to this state was initiated. + p._currentState.Dispose(p); + } + else if (fromState != Exited) + { + Kill(p); + + p._exitCompletionSource.TrySetException(new ObjectDisposedException(p.ToString())); + p._tempUserDataDir?.Dispose(); + } + } + + public override Task StartAsync(ChromiumProcess p) => throw new ObjectDisposedException(p.ToString()); + + public override Task ExitAsync(ChromiumProcess p, TimeSpan timeout) => throw new ObjectDisposedException(p.ToString()); + + public override Task KillAsync(ChromiumProcess p) => throw new ObjectDisposedException(p.ToString()); + + public override void Dispose(ChromiumProcess p) + { + // Nothing to do + } + } + + #endregion + } + + #endregion + } +} \ No newline at end of file diff --git a/lib/PuppeteerSharp/ChromiumProcessException.cs b/lib/PuppeteerSharp/ChromiumProcessException.cs new file mode 100644 index 000000000..0dd54a36d --- /dev/null +++ b/lib/PuppeteerSharp/ChromiumProcessException.cs @@ -0,0 +1,27 @@ +using System; + +namespace PuppeteerSharp +{ + /// + /// Chromium process exception thrown by . + /// + public class ChromiumProcessException : PuppeteerException + { + /// + /// Initializes a new instance of the class. + /// + /// Message. + public ChromiumProcessException(string message) : base(message) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// Message. + /// Inner exception. + public ChromiumProcessException(string message, Exception innerException) : base(message, innerException) + { + } + } +} \ No newline at end of file diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs index c24a97bbd..53cf95fba 100755 --- a/lib/PuppeteerSharp/Connection.cs +++ b/lib/PuppeteerSharp/Connection.cs @@ -123,7 +123,7 @@ public async Task SendAsync(string method, dynamic args = null) internal async Task SendAsync(string method, dynamic args = null) { - JToken response = await SendAsync(method, args); + JToken response = await SendAsync(method, args).ConfigureAwait(false); return response.ToObject(); } diff --git a/lib/PuppeteerSharp/Helpers/TempDirectory.cs b/lib/PuppeteerSharp/Helpers/TempDirectory.cs new file mode 100644 index 000000000..712cc0bfc --- /dev/null +++ b/lib/PuppeteerSharp/Helpers/TempDirectory.cs @@ -0,0 +1,87 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using PathHelper = System.IO.Path; + +namespace PuppeteerSharp.Helpers +{ + /// + /// Represents a directory that is deleted on disposal. + /// + internal class TempDirectory : IDisposable + { + private Task _deleteTask; + + public TempDirectory() + : this(PathHelper.Combine(PathHelper.GetTempPath(), PathHelper.GetRandomFileName())) + { } + + public TempDirectory(string path) + { + if (string.IsNullOrEmpty(path)) + { + throw new ArgumentException("Path must be specified", nameof(path)); + } + + Directory.CreateDirectory(path); + this.Path = path; + } + + ~TempDirectory() + { + Dispose(false); + } + + public string Path { get; } + + public void Dispose() + { + GC.SuppressFinalize(this); + Dispose(true); + } + + protected void Dispose(bool disposing) + { + if (_deleteTask == null) + { + _ = DeleteAsync(); + } + } + + public override string ToString() => Path; + + public Task DeleteAsync(CancellationToken cancellationToken = default) + => _deleteTask ?? (_deleteTask = DeleteAsync(Path, CancellationToken.None)); + + private static async Task DeleteAsync(string path, CancellationToken cancellationToken = default) + { + const int minDelayInMsec = 200; + const int maxDelayInMsec = 8000; + + var retryDelay = minDelayInMsec; + while (true) + { + if (!Directory.Exists(path)) + { + return; + } + + cancellationToken.ThrowIfCancellationRequested(); + try + { + Directory.Delete(path, true); + return; + } + catch + { + await Task.Delay(retryDelay, cancellationToken).ConfigureAwait(false); + if (retryDelay < maxDelayInMsec) + { + retryDelay = Math.Min(2 * retryDelay, maxDelayInMsec); + } + } + } + } + } +} diff --git a/lib/PuppeteerSharp/Launcher.cs b/lib/PuppeteerSharp/Launcher.cs index 0714f026e..3e4560b38 100644 --- a/lib/PuppeteerSharp/Launcher.cs +++ b/lib/PuppeteerSharp/Launcher.cs @@ -1,13 +1,7 @@ using System; -using System.Collections.Generic; using System.Threading.Tasks; -using PuppeteerSharp.Helpers; using System.Linq; using System.IO; -using System.Diagnostics; -using System.Text.RegularExpressions; -using System.Threading; -using System.Collections; using Microsoft.Extensions.Logging; using PuppeteerSharp.Messaging; @@ -18,54 +12,11 @@ namespace PuppeteerSharp /// public class Launcher { - #region Constants - internal static readonly string[] DefaultArgs = { - "--disable-background-networking", - "--disable-background-timer-throttling", - "--disable-breakpad", - "--disable-client-side-phishing-detection", - "--disable-default-apps", - "--disable-dev-shm-usage", - "--disable-extensions", - "--disable-features=site-per-process", - "--disable-hang-monitor", - "--disable-popup-blocking", - "--disable-prompt-on-repost", - "--disable-sync", - "--disable-translate", - "--metrics-recording-only", - "--no-first-run", - "--safebrowsing-disable-auto-update" - }; - internal static readonly string[] AutomationArgs = { - "--enable-automation", - "--password-store=basic", - "--use-mock-keychain" - }; - private const string UserDataDirArgument = "--user-data-dir"; - #endregion - #region Private members - private static int _processCount; + private readonly ILoggerFactory _loggerFactory; - private readonly ILogger _logger; - private Process _chromeProcess; - private string _temporaryUserDataDir; - private Connection _connection; - private Timer _timer; - private LaunchOptions _options; - private TaskCompletionSource _waitForChromeToClose; - private bool _processLoaded; private bool _chromiumLaunched; - private object _isChromeCloseLock = new object(); - #endregion - #region Properties - /// - /// Gets or sets a value indicating whether the process created by the instance is closed. - /// - /// true if is the process is closed; otherwise, false. - public bool IsChromeClosed { get; internal set; } #endregion /// @@ -73,13 +24,17 @@ public class Launcher /// /// Logger factory. public Launcher(ILoggerFactory loggerFactory = null) - { _loggerFactory = loggerFactory ?? new LoggerFactory(); - _logger = _loggerFactory.CreateLogger(); - _waitForChromeToClose = new TaskCompletionSource(); } + #region Properties + /// + /// Gets Chromium process, if any was created by this launcher. + /// + public ChromiumProcess Process { get; private set; } + #endregion + #region Public methods /// /// The method launches a browser instance with given arguments. The browser will be closed when the Browser is disposed. @@ -93,56 +48,36 @@ public Launcher(ILoggerFactory loggerFactory = null) /// public async Task LaunchAsync(LaunchOptions options) { - if (_chromiumLaunched) - { - throw new InvalidOperationException("Unable to create or connect to another chromium process"); - } - _chromiumLaunched = true; - var chromeArguments = InitChromeArgument(options); - var chromeExecutable = options.ExecutablePath; - - if (string.IsNullOrEmpty(chromeExecutable)) - { - var browserFetcher = new BrowserFetcher(); - chromeExecutable = browserFetcher.RevisionInfo(BrowserFetcher.DefaultRevision).ExecutablePath; - } - if (!File.Exists(chromeExecutable)) - { - throw new FileNotFoundException("Failed to launch chrome! path to executable does not exist", chromeExecutable); - } - - CreateChromeProcess(options, chromeArguments, chromeExecutable); + EnsureSingleLaunchOrConnect(); + var chromiumExecutable = GetOrFetchChromeExecutable(options); + Process = new ChromiumProcess(chromiumExecutable, options, _loggerFactory); try { - var connectionDelay = options.SlowMo; - var browserWSEndpoint = await WaitForEndpoint(_chromeProcess, options.Timeout).ConfigureAwait(false); - var keepAliveInterval = 0; + await Process.StartAsync().ConfigureAwait(false); + try + { + var keepAliveInterval = 0; + var connection = await Connection + .Create(Process.EndPoint, options.SlowMo, keepAliveInterval, _loggerFactory) + .ConfigureAwait(false); - _connection = await Connection.Create(browserWSEndpoint, connectionDelay, keepAliveInterval, _loggerFactory).ConfigureAwait(false); - _processLoaded = true; + var browser = await Browser + .CreateAsync(connection, Array.Empty(), options.IgnoreHTTPSErrors, !options.AppMode, Process) + .ConfigureAwait(false); - if (options.LogProcess) + await EnsureInitialPageAsync(browser).ConfigureAwait(false); + return browser; + } + catch (Exception ex) { - _logger.LogInformation("Process Count: {ProcessCount}", Interlocked.Increment(ref _processCount)); + throw new ChromiumProcessException("Failed to create connection", ex); } - - var browser = await Browser.CreateAsync( - _connection, - Array.Empty(), - options.IgnoreHTTPSErrors, - !options.AppMode, - _chromeProcess, - GracefullyCloseChrome - ).ConfigureAwait(false); - - await EnsureInitialPageAsync(browser).ConfigureAwait(false); - return browser; } - catch (Exception ex) + catch { - KillChrome(); - throw new ChromeProcessException("Failed to create connection", ex); + await Process.KillAsync().ConfigureAwait(false); + throw; } } @@ -153,82 +88,20 @@ public async Task LaunchAsync(LaunchOptions options) /// A connected browser. public async Task ConnectAsync(ConnectOptions options) { + EnsureSingleLaunchOrConnect(); + try { - if (_chromiumLaunched) - { - throw new InvalidOperationException("Unable to create or connect to another chromium process"); - } - _chromiumLaunched = true; - - var connectionDelay = options.SlowMo; var keepAliveInterval = 0; - - _connection = await Connection.Create(options.BrowserWSEndpoint, connectionDelay, keepAliveInterval, _loggerFactory).ConfigureAwait(false); - - var response = await _connection.SendAsync("Target.getBrowserContexts"); - - return await Browser.CreateAsync(_connection, response.BrowserContextIds, options.IgnoreHTTPSErrors, true, null, () => - { - try - { - var closeTask = _connection.SendAsync("Browser.close", null); - } - catch (Exception ex) - { - _logger.LogError(ex, ex.Message); - } - return null; - }).ConfigureAwait(false); + var connection = await Connection.Create(options.BrowserWSEndpoint, options.SlowMo, keepAliveInterval, _loggerFactory).ConfigureAwait(false); + var response = await connection.SendAsync("Target.getBrowserContexts"); + return await Browser + .CreateAsync(connection, response.BrowserContextIds, options.IgnoreHTTPSErrors, true, null) + .ConfigureAwait(false); } catch (Exception ex) { - throw new Exception("Failed to create connection", ex); - } - } - - /// - /// Tries the delete user data dir. - /// - /// The task. - /// How many times it should try to delete the folder - /// Time to wait between tries. - public async Task TryDeleteUserDataDir(int times = 10, TimeSpan? delay = null) - { - if (!IsChromeClosed) - { - throw new InvalidOperationException("Unable to delete user data dir, Chorme is still open"); - } - - if (times <= 0) - { - throw new ArgumentOutOfRangeException(nameof(times)); - } - - if (delay == null) - { - delay = new TimeSpan(0, 0, 0, 0, 100); - } - - var folder = string.IsNullOrEmpty(_temporaryUserDataDir) ? _options.UserDataDir : _temporaryUserDataDir; - var attempts = 0; - while (true) - { - try - { - attempts++; - Directory.Delete(folder, true); - break; - } - catch (UnauthorizedAccessException) - { - if (attempts == times) - { - throw; - } - - await Task.Delay(delay.Value).ConfigureAwait(false); - } + throw new ChromiumProcessException("Failed to create connection", ex); } } @@ -239,281 +112,57 @@ public async Task TryDeleteUserDataDir(int times = 10, TimeSpan? delay = null) public static string GetExecutablePath() => new BrowserFetcher().RevisionInfo(BrowserFetcher.DefaultRevision).ExecutablePath; - /// - /// Gets a temporary directory using and . - /// - /// A temporary directory. - public static string GetTemporaryDirectory() - { - var tempDirectory = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); - Directory.CreateDirectory(tempDirectory); - return tempDirectory; - } - #endregion #region Private methods - private static Task EnsureInitialPageAsync(Browser browser) - { - // Wait for initial page target to be created. - if (browser.Targets().Any(target => target.Type == TargetType.Page)) - { - return Task.CompletedTask; - } - var initialPageCompletion = new TaskCompletionSource(); - void InitialPageCallback(object sender, TargetChangedArgs e) - { - if (e.Target.Type == TargetType.Page) - { - initialPageCompletion.SetResult(true); - browser.TargetCreated -= InitialPageCallback; - } - } - browser.TargetCreated += InitialPageCallback; - return initialPageCompletion.Task; - } - - private void CreateChromeProcess(LaunchOptions options, List chromeArguments, string chromeExecutable) + private void EnsureSingleLaunchOrConnect() { - _chromeProcess = new Process - { - EnableRaisingEvents = true - }; - _chromeProcess.StartInfo.UseShellExecute = false; - _chromeProcess.StartInfo.FileName = chromeExecutable; - _chromeProcess.StartInfo.Arguments = string.Join(" ", chromeArguments); - _chromeProcess.StartInfo.RedirectStandardError = true; - - SetEnvVariables(_chromeProcess.StartInfo.Environment, options.Env, Environment.GetEnvironmentVariables()); - - _chromeProcess.Exited += async (sender, e) => - { - await AfterProcessExit().ConfigureAwait(false); - }; - - _chromeProcess.ErrorDataReceived += (sender, e) => - { - if (options.DumpIO) - { - Console.Error.WriteLine(e.Data); - } - }; - } - - private List InitChromeArgument(LaunchOptions options) - { - var chromeArguments = new List(DefaultArgs); - - _options = options; - - if (options.AppMode) - { - options.Headless = false; - } - else - { - chromeArguments.AddRange(AutomationArgs); - } - - if (!options.IgnoreDefaultArgs || - !chromeArguments.Any(argument => argument.StartsWith("--remote-debugging-", StringComparison.Ordinal))) - { - chromeArguments.Add("--remote-debugging-port=0"); - } - - var userDataDirOption = options.Args.FirstOrDefault(i => i.StartsWith(UserDataDirArgument, StringComparison.Ordinal)); - if (string.IsNullOrEmpty(userDataDirOption)) - { - if (string.IsNullOrEmpty(options.UserDataDir)) - { - _temporaryUserDataDir = GetTemporaryDirectory(); - chromeArguments.Add($"{UserDataDirArgument}={_temporaryUserDataDir.Quote()}"); - } - else - { - chromeArguments.Add($"{UserDataDirArgument}={options.UserDataDir.Quote()}"); - } - } - else - { - _options.UserDataDir = userDataDirOption.Replace($"{UserDataDirArgument}=", string.Empty).UnQuote(); - } - - if (options.Devtools) - { - chromeArguments.Add("--auto-open-devtools-for-tabs"); - options.Headless = false; - } - - if (options.Headless) - { - chromeArguments.AddRange(new[]{ - "--headless", - "--disable-gpu", - "--hide-scrollbars", - "--mute-audio" - }); - } - - if (!options.IgnoreDefaultArgs && options.Args.Any() && options.Args.All(arg => arg.StartsWith("-"))) - { - chromeArguments.Add("about:blank"); - } - - if (options.Args.Any()) - { - chromeArguments.AddRange(options.Args); - } - - return chromeArguments; - } - - private Task WaitForEndpoint(Process chromeProcess, int timeout) - { - var taskWrapper = new TaskCompletionSource(); - var output = string.Empty; - - void exitedEvent(object sender, EventArgs e) - { - if (_options.LogProcess && !_processLoaded) - { - _logger.LogInformation("Process Count: {ProcessCount}", Interlocked.Increment(ref _processCount)); - } - - CleanUp(); - - taskWrapper.SetException(new ChromeProcessException($"Failed to launch chrome! {output}")); - } - - void errorDataReceivedEvent(object sender, DataReceivedEventArgs e) - { - if (e.Data != null) - { - output += e.Data + "\n"; - var match = Regex.Match(e.Data, "^DevTools listening on (ws:\\/\\/.*)"); - - if (!match.Success) - { - return; - } - - CleanUp(); - chromeProcess.Exited -= exitedEvent; - chromeProcess.ErrorDataReceived -= errorDataReceivedEvent; - taskWrapper.SetResult(match.Groups[1].Value); - } - } - - chromeProcess.ErrorDataReceived += errorDataReceivedEvent; - chromeProcess.Exited += exitedEvent; - - if (timeout > 0) + if (_chromiumLaunched) { - //We have to declare timer before initializing it because if we don't do this - //we can't dispose it in the action created in the constructor - _timer = new Timer((state) => - { - taskWrapper.SetException( - new ChromeProcessException($"Timed out after {timeout} ms while trying to connect to Chrome! ")); - _timer.Dispose(); - }, null, timeout, 0); + throw new InvalidOperationException("Unable to create or connect to another chromium process"); } - chromeProcess.Start(); - chromeProcess.BeginErrorReadLine(); - return taskWrapper.Task; - } - - private void CleanUp() - { - _timer?.Dispose(); - _timer = null; - _chromeProcess?.RemoveExitedEvent(); + _chromiumLaunched = true; } - private async Task AfterProcessExit() + private static string GetOrFetchChromeExecutable(LaunchOptions options) { - lock (_isChromeCloseLock) - { - if (IsChromeClosed) - { - return; - } - IsChromeClosed = true; - } - if (_options.LogProcess) - { - _logger.LogInformation("Process Count: {ProcessCount}", Interlocked.Decrement(ref _processCount)); - } - - if (_temporaryUserDataDir != null) - { - await TryDeleteUserDataDir().ConfigureAwait(false); - } - - if (_waitForChromeToClose.Task.Status != TaskStatus.RanToCompletion) + var chromeExecutable = options.ExecutablePath; + if (string.IsNullOrEmpty(chromeExecutable)) { - _waitForChromeToClose.SetResult(true); + var browserFetcher = new BrowserFetcher(); + chromeExecutable = browserFetcher.RevisionInfo(BrowserFetcher.DefaultRevision).ExecutablePath; } - } - private async Task GracefullyCloseChrome() - { - if (!string.IsNullOrEmpty(_temporaryUserDataDir)) - { - KillChrome(); - await AfterProcessExit().ConfigureAwait(false); - } - else if (_connection != null) + if (!File.Exists(chromeExecutable)) { - try - { - await _connection.SendAsync("Browser.close", null).ConfigureAwait(false); - } - catch (Exception ex) - { - _logger.LogError(ex, ex.Message); - KillChrome(); - } + throw new FileNotFoundException("Failed to launch chrome! path to executable does not exist", chromeExecutable); } - await _waitForChromeToClose.Task.ConfigureAwait(false); - } - - private void KillChrome() - { - try - { - if (_chromeProcess.Id != 0 && !_chromeProcess.HasExited && Process.GetProcessById(_chromeProcess.Id) != null) - { - _chromeProcess.Kill(); - _chromeProcess.WaitForExit(); - } - } - catch (InvalidOperationException ex) when (ex.Message == "No process is associated with this object.") - { - // swallow - } + return chromeExecutable; } - private static void SetEnvVariables(IDictionary environment, IDictionary customEnv, - IDictionary realEnv) + private static Task EnsureInitialPageAsync(Browser browser) { - foreach (DictionaryEntry item in realEnv) + // Wait for initial page target to be created. + if (browser.Targets().Any(target => target.Type == TargetType.Page)) { - environment[item.Key.ToString()] = item.Value.ToString(); + return Task.CompletedTask; } - - if (customEnv != null) + var initialPageCompletion = new TaskCompletionSource(); + void InitialPageCallback(object sender, TargetChangedArgs e) { - foreach (var item in customEnv) + if (e.Target.Type == TargetType.Page) { - environment[item.Key] = item.Value; + initialPageCompletion.SetResult(true); + browser.TargetCreated -= InitialPageCallback; } } + browser.TargetCreated += InitialPageCallback; + return initialPageCompletion.Task; } #endregion } -} \ No newline at end of file +} diff --git a/lib/PuppeteerSharp/Puppeteer.cs b/lib/PuppeteerSharp/Puppeteer.cs index a2af2ec32..b722d46a8 100644 --- a/lib/PuppeteerSharp/Puppeteer.cs +++ b/lib/PuppeteerSharp/Puppeteer.cs @@ -20,7 +20,7 @@ public static class Puppeteer /// /// The default flags that Chromium will be launched with. /// - public static string[] DefaultArgs => Launcher.DefaultArgs; + public static string[] DefaultArgs => ChromiumProcess.DefaultArgs; /// /// A path where Puppeteer expects to find bundled Chromium. Chromium might not exist there if the downloader was not used. From f6376a267e4814508fc9494779f67143093c80ef Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Wed, 5 Sep 2018 00:28:43 +0200 Subject: [PATCH 6/9] Solve race conditions in Connection response processing --- lib/PuppeteerSharp/Browser.cs | 1 - lib/PuppeteerSharp/CDPSession.cs | 65 +++++----- lib/PuppeteerSharp/Connection.cs | 137 +++++++++----------- lib/PuppeteerSharp/TargetClosedException.cs | 13 +- 4 files changed, 103 insertions(+), 113 deletions(-) diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index 9420b9ebb..c22b065ea 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -239,7 +239,6 @@ public async Task GetUserAgentAsync() private async Task CloseCoreAsync() { - Connection.StopReading(); try { try diff --git a/lib/PuppeteerSharp/CDPSession.cs b/lib/PuppeteerSharp/CDPSession.cs index 684c5f890..9c028d153 100755 --- a/lib/PuppeteerSharp/CDPSession.cs +++ b/lib/PuppeteerSharp/CDPSession.cs @@ -1,10 +1,10 @@ using System; +using System.Collections.Concurrent; +using System.Collections.Generic; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; using Newtonsoft.Json; -using System.Collections.Generic; using Newtonsoft.Json.Linq; -using Microsoft.Extensions.Logging; -using PuppeteerSharp.Helpers; namespace PuppeteerSharp { @@ -43,16 +43,14 @@ internal CDPSession(IConnection connection, TargetType targetType, string sessio TargetType = targetType; SessionId = sessionId; - _callbacks = new Dictionary(); _logger = Connection.LoggerFactory.CreateLogger(); - _sessions = new Dictionary(); } #region Private Members private int _lastId; - private readonly Dictionary _callbacks; + private readonly ConcurrentDictionary _callbacks = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _sessions = new ConcurrentDictionary(); private readonly ILogger _logger; - private readonly Dictionary _sessions; #endregion #region Properties @@ -79,11 +77,12 @@ internal CDPSession(IConnection connection, TargetType targetType, string sessio /// Occurs when tracing is completed. /// public event EventHandler TracingComplete; + /// /// Gets or sets a value indicating whether this is closed. /// /// true if is closed; otherwise, false. - public bool IsClosed { get; internal set; } + public bool IsClosed => Connection == null; /// /// Gets the logger factory. @@ -111,6 +110,7 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a { throw new Exception($"Protocol error ({method}): Session closed. Most likely the {TargetType} has been closed."); } + var id = ++_lastId; var message = JsonConvert.SerializeObject(new Dictionary { @@ -126,8 +126,7 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a Method = method, RawContent = rawContent }; - - _callbacks[id] = callback; + _callbacks.TryAdd(id, callback); try { @@ -139,10 +138,9 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a } catch (Exception ex) { - if (_callbacks.ContainsKey(id)) + if (_callbacks.TryRemove(id, out _)) { - _callbacks.Remove(id); - callback.TaskWrapper.SetException(new MessageException(ex.Message, ex)); + callback.TaskWrapper.TrySetException(new MessageException(ex.Message, ex)); } } @@ -163,18 +161,16 @@ public Task DetachAsync() internal void OnMessage(string message) { dynamic obj = JsonConvert.DeserializeObject(message); - var objAsJObject = obj as JObject; + var objAsJObject = (JObject)obj; _logger.LogTrace("◀ Receive {Message}", message); - if (objAsJObject["id"] != null && _callbacks.ContainsKey((int)obj.id)) + var id = (int?)objAsJObject["id"]; + if (id.HasValue && _callbacks.TryRemove(id.Value, out var callback)) { - var callback = _callbacks[(int)obj.id]; - _callbacks.Remove((int)obj.id); - if (objAsJObject["error"] != null) { - callback.TaskWrapper.SetException(new MessageException( + callback.TaskWrapper.TrySetException(new MessageException( $"Protocol error ({ callback.Method }): {obj.error.message} {obj.error.data}" )); } @@ -182,11 +178,11 @@ internal void OnMessage(string message) { if (callback.RawContent) { - callback.TaskWrapper.SetResult(JsonConvert.SerializeObject(obj.result)); + callback.TaskWrapper.TrySetResult(JsonConvert.SerializeObject(obj.result)); } else { - callback.TaskWrapper.SetResult(obj.result); + callback.TaskWrapper.TrySetResult(obj.result); } } } @@ -201,19 +197,16 @@ internal void OnMessage(string message) } else if (obj.method == "Target.receivedMessageFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (session != null) + if (_sessions.TryGetValue(objAsJObject["params"]["sessionId"].ToString(), out var session)) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (!(session?.IsClosed ?? true)) + if (_sessions.TryRemove(objAsJObject["params"]["sessionId"].ToString(), out var session) && !session.IsClosed) { session.OnClosed(); - _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } @@ -227,24 +220,30 @@ internal void OnMessage(string message) internal void OnClosed() { - IsClosed = true; - foreach (var callback in _callbacks.Values) + if (Connection == null) { - callback.TaskWrapper.SetException(new TargetClosedException( - $"Protocol error({callback.Method}): Target closed." - )); + return; } - _callbacks.Clear(); Connection = null; + + foreach (var entry in _callbacks) + { + if (_callbacks.TryRemove(entry.Key, out _)) + { + entry.Value.TaskWrapper.TrySetException( + new TargetClosedException($"Protocol error({entry.Value.Method}): Target closed.")); + } + } } internal CDPSession CreateSession(TargetType targetType, string sessionId) { var session = new CDPSession(this, targetType, sessionId); - _sessions[sessionId] = session; + _sessions.TryAdd(sessionId, session); return session; } #endregion + #region IConnection ILoggerFactory IConnection.LoggerFactory => LoggerFactory; bool IConnection.IsClosed => IsClosed; diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs index c807e1f13..5622c5726 100755 --- a/lib/PuppeteerSharp/Connection.cs +++ b/lib/PuppeteerSharp/Connection.cs @@ -1,6 +1,6 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; -using System.Linq; using System.Net.WebSockets; using System.Text; using System.Threading; @@ -27,22 +27,16 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa WebSocket = ws; _logger = LoggerFactory.CreateLogger(); - _socketQueue = new TaskQueue(); - _responses = new Dictionary(); - _sessions = new Dictionary(); - _websocketReaderCancellationSource = new CancellationTokenSource(); Task.Factory.StartNew(GetResponseAsync); } #region Private Members private int _lastId; - private Dictionary _responses; - private Dictionary _sessions; - private TaskQueue _socketQueue; - private const string CloseMessage = "Browser.close"; - private bool _stopReading; - private CancellationTokenSource _websocketReaderCancellationSource; + private readonly ConcurrentDictionary _responses = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _sessions = new ConcurrentDictionary(); + private readonly TaskQueue _socketQueue = new TaskQueue(); + private readonly CancellationTokenSource _websocketReaderCancellationSource = new CancellationTokenSource(); #endregion #region Properties @@ -50,17 +44,17 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa /// Gets the WebSocket URL. /// /// The URL. - public string Url { get; private set; } + public string Url { get; } /// /// Gets the sleep time when a message is received. /// /// The delay. - public int Delay { get; private set; } + public int Delay { get; } /// /// Gets the WebSocket. /// /// The web socket. - public WebSocket WebSocket { get; private set; } + public WebSocket WebSocket { get; } /// /// Occurs when the connection is closed. /// @@ -102,23 +96,28 @@ internal async Task SendAsync(string method, dynamic args = null) _logger.LogTrace("Send ► {Id} Method {Method} Params {@Params}", id, method, (object)args); - var taskWrapper = new TaskCompletionSource(); - _responses[id] = new MessageTask + var callback = new MessageTask { - TaskWrapper = taskWrapper, + TaskWrapper = new TaskCompletionSource(), Method = method }; + _responses.TryAdd(id, callback); - var encoded = Encoding.UTF8.GetBytes(message); - var buffer = new ArraySegment(encoded, 0, encoded.Length); - await _socketQueue.Enqueue(() => WebSocket.SendAsync(buffer, WebSocketMessageType.Text, true, default)).ConfigureAwait(false); - - if (method == CloseMessage) + try + { + var encoded = Encoding.UTF8.GetBytes(message); + var buffer = new ArraySegment(encoded, 0, encoded.Length); + await _socketQueue.Enqueue(() => WebSocket.SendAsync(buffer, WebSocketMessageType.Text, true, default)).ConfigureAwait(false); + } + catch (Exception ex) { - StopReading(); + if (_responses.TryRemove(id, out _)) + { + callback.TaskWrapper.TrySetException(ex); + } } - return await taskWrapper.Task.ConfigureAwait(false); + return await callback.TaskWrapper.Task.ConfigureAwait(false); } internal async Task SendAsync(string method, dynamic args = null) @@ -134,41 +133,40 @@ internal async Task CreateSessionAsync(TargetInfo targetInfo) targetId = targetInfo.TargetId }).ConfigureAwait(false)).sessionId; var session = new CDPSession(this, targetInfo.Type, sessionId); - _sessions.Add(sessionId, session); + _sessions.TryAdd(sessionId, session); return session; } #endregion - private void OnClose() + private void OnClose(Exception ex) { if (IsClosed) { return; } - IsClosed = true; _websocketReaderCancellationSource.Cancel(); Closed?.Invoke(this, new EventArgs()); - foreach (var session in _sessions.Values) + foreach (var entry in _sessions) { - session.OnClosed(); + if (_sessions.TryRemove(entry.Key, out _)) + { + entry.Value.OnClosed(); + } } - foreach (var response in _responses.Values.Where(r => !r.TaskWrapper.Task.IsCompleted)) + foreach (var entry in _responses) { - response.TaskWrapper.SetException(new TargetClosedException( - $"Protocol error({response.Method}): Target closed." - )); + if (_responses.TryRemove(entry.Key, out _)) + { + entry.Value.TaskWrapper.TrySetException( + new TargetClosedException($"Protocol error({entry.Value.Method}): Target closed.", ex)); + } } - - _responses.Clear(); - _sessions.Clear(); } - internal void StopReading() => _stopReading = true; - #region Private Methods /// @@ -180,105 +178,86 @@ private async Task GetResponseAsync() var buffer = new byte[2048]; //If it's not in the list we wait for it - while (true) + while (!IsClosed) { - if (IsClosed) - { - OnClose(); - return null; - } - var endOfMessage = false; - var response = string.Empty; + var response = new StringBuilder(); while (!endOfMessage) { - WebSocketReceiveResult result = null; + WebSocketReceiveResult result; try { result = await WebSocket.ReceiveAsync( new ArraySegment(buffer), _websocketReaderCancellationSource.Token).ConfigureAwait(false); } - catch (Exception) when (_stopReading) - { - return null; - } catch (OperationCanceledException) { return null; } - catch (Exception) + catch (Exception ex) { - if (!IsClosed) - { - OnClose(); - return null; - } + OnClose(ex); + return null; } endOfMessage = result.EndOfMessage; if (result.MessageType == WebSocketMessageType.Text) { - response += Encoding.UTF8.GetString(buffer, 0, result.Count); + response.Append(Encoding.UTF8.GetString(buffer, 0, result.Count)); } else if (result.MessageType == WebSocketMessageType.Close) { - OnClose(); + OnClose(null); return null; } } - if (!string.IsNullOrEmpty(response)) + if (response.Length > 0) { if (Delay > 0) { await Task.Delay(Delay).ConfigureAwait(false); } - ProcessResponse(response); + ProcessResponse(response.ToString()); } } + + return null; } private void ProcessResponse(string response) { dynamic obj = JsonConvert.DeserializeObject(response); - var objAsJObject = obj as JObject; + var objAsJObject = (JObject)obj; _logger.LogTrace("◀ Receive {Message}", response); - if (objAsJObject["id"] != null) + var id = (int?)objAsJObject["id"]; + if (id.HasValue) { - var id = (int)objAsJObject["id"]; - - //If we get the object we are waiting for we return if - //if not we add this to the list, sooner or later some one will come for it - if (!_responses.ContainsKey(id)) + if (_responses.TryRemove(id.Value, out var callback)) { - _responses[id] = new MessageTask { TaskWrapper = new TaskCompletionSource() }; + callback.TaskWrapper.TrySetResult(obj.result); } - - _responses[id].TaskWrapper.SetResult(obj.result); } else { if (obj.method == "Target.receivedMessageFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (session != null) + if (_sessions.TryGetValue(objAsJObject["params"]["sessionId"].ToString(), out var session)) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (!(session?.IsClosed ?? true)) + if (_sessions.TryRemove(objAsJObject["params"]["sessionId"].ToString(), out var session) && !session.IsClosed) { session.OnClosed(); - _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } else @@ -286,12 +265,14 @@ private void ProcessResponse(string response) MessageReceived?.Invoke(this, new MessageEventArgs { MessageID = obj.method, - MessageData = objAsJObject["params"] as dynamic + MessageData = objAsJObject["params"] }); } } } + #endregion + #region Static Methods /// @@ -322,7 +303,7 @@ internal static async Task Create(string url, IConnectionOptions con /// was occupying. public void Dispose() { - OnClose(); + OnClose(new ObjectDisposedException($"Connection({Url})")); WebSocket.Dispose(); } #endregion diff --git a/lib/PuppeteerSharp/TargetClosedException.cs b/lib/PuppeteerSharp/TargetClosedException.cs index 10811b0d8..23a319c58 100644 --- a/lib/PuppeteerSharp/TargetClosedException.cs +++ b/lib/PuppeteerSharp/TargetClosedException.cs @@ -1,4 +1,6 @@ -namespace PuppeteerSharp +using System; + +namespace PuppeteerSharp { /// /// Exception thrown by the when it detects that the target was closed. @@ -12,5 +14,14 @@ public class TargetClosedException : PuppeteerException public TargetClosedException(string message) : base(message) { } + + /// + /// Initializes a new instance of the class. + /// + /// Message. + /// Inner exception. + public TargetClosedException(string message, Exception innerException) : base(message, innerException) + { + } } } \ No newline at end of file From 142f91253aede12e8ac2be778d2fd738a3faf518 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Wed, 5 Sep 2018 15:42:46 +0200 Subject: [PATCH 7/9] Revert "Solve race conditions in Connection response processing" This reverts commit f6376a267e4814508fc9494779f67143093c80ef. --- lib/PuppeteerSharp/Browser.cs | 1 + lib/PuppeteerSharp/CDPSession.cs | 65 +++++----- lib/PuppeteerSharp/Connection.cs | 137 +++++++++++--------- lib/PuppeteerSharp/TargetClosedException.cs | 13 +- 4 files changed, 113 insertions(+), 103 deletions(-) diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index c22b065ea..9420b9ebb 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -239,6 +239,7 @@ public async Task GetUserAgentAsync() private async Task CloseCoreAsync() { + Connection.StopReading(); try { try diff --git a/lib/PuppeteerSharp/CDPSession.cs b/lib/PuppeteerSharp/CDPSession.cs index 9c028d153..684c5f890 100755 --- a/lib/PuppeteerSharp/CDPSession.cs +++ b/lib/PuppeteerSharp/CDPSession.cs @@ -1,10 +1,10 @@ using System; -using System.Collections.Concurrent; -using System.Collections.Generic; using System.Threading.Tasks; -using Microsoft.Extensions.Logging; using Newtonsoft.Json; +using System.Collections.Generic; using Newtonsoft.Json.Linq; +using Microsoft.Extensions.Logging; +using PuppeteerSharp.Helpers; namespace PuppeteerSharp { @@ -43,14 +43,16 @@ internal CDPSession(IConnection connection, TargetType targetType, string sessio TargetType = targetType; SessionId = sessionId; + _callbacks = new Dictionary(); _logger = Connection.LoggerFactory.CreateLogger(); + _sessions = new Dictionary(); } #region Private Members private int _lastId; - private readonly ConcurrentDictionary _callbacks = new ConcurrentDictionary(); - private readonly ConcurrentDictionary _sessions = new ConcurrentDictionary(); + private readonly Dictionary _callbacks; private readonly ILogger _logger; + private readonly Dictionary _sessions; #endregion #region Properties @@ -77,12 +79,11 @@ internal CDPSession(IConnection connection, TargetType targetType, string sessio /// Occurs when tracing is completed. /// public event EventHandler TracingComplete; - /// /// Gets or sets a value indicating whether this is closed. /// /// true if is closed; otherwise, false. - public bool IsClosed => Connection == null; + public bool IsClosed { get; internal set; } /// /// Gets the logger factory. @@ -110,7 +111,6 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a { throw new Exception($"Protocol error ({method}): Session closed. Most likely the {TargetType} has been closed."); } - var id = ++_lastId; var message = JsonConvert.SerializeObject(new Dictionary { @@ -126,7 +126,8 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a Method = method, RawContent = rawContent }; - _callbacks.TryAdd(id, callback); + + _callbacks[id] = callback; try { @@ -138,9 +139,10 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a } catch (Exception ex) { - if (_callbacks.TryRemove(id, out _)) + if (_callbacks.ContainsKey(id)) { - callback.TaskWrapper.TrySetException(new MessageException(ex.Message, ex)); + _callbacks.Remove(id); + callback.TaskWrapper.SetException(new MessageException(ex.Message, ex)); } } @@ -161,16 +163,18 @@ public Task DetachAsync() internal void OnMessage(string message) { dynamic obj = JsonConvert.DeserializeObject(message); - var objAsJObject = (JObject)obj; + var objAsJObject = obj as JObject; _logger.LogTrace("◀ Receive {Message}", message); - var id = (int?)objAsJObject["id"]; - if (id.HasValue && _callbacks.TryRemove(id.Value, out var callback)) + if (objAsJObject["id"] != null && _callbacks.ContainsKey((int)obj.id)) { + var callback = _callbacks[(int)obj.id]; + _callbacks.Remove((int)obj.id); + if (objAsJObject["error"] != null) { - callback.TaskWrapper.TrySetException(new MessageException( + callback.TaskWrapper.SetException(new MessageException( $"Protocol error ({ callback.Method }): {obj.error.message} {obj.error.data}" )); } @@ -178,11 +182,11 @@ internal void OnMessage(string message) { if (callback.RawContent) { - callback.TaskWrapper.TrySetResult(JsonConvert.SerializeObject(obj.result)); + callback.TaskWrapper.SetResult(JsonConvert.SerializeObject(obj.result)); } else { - callback.TaskWrapper.TrySetResult(obj.result); + callback.TaskWrapper.SetResult(obj.result); } } } @@ -197,16 +201,19 @@ internal void OnMessage(string message) } else if (obj.method == "Target.receivedMessageFromTarget") { - if (_sessions.TryGetValue(objAsJObject["params"]["sessionId"].ToString(), out var session)) + var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); + if (session != null) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - if (_sessions.TryRemove(objAsJObject["params"]["sessionId"].ToString(), out var session) && !session.IsClosed) + var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); + if (!(session?.IsClosed ?? true)) { session.OnClosed(); + _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } @@ -220,30 +227,24 @@ internal void OnMessage(string message) internal void OnClosed() { - if (Connection == null) + IsClosed = true; + foreach (var callback in _callbacks.Values) { - return; + callback.TaskWrapper.SetException(new TargetClosedException( + $"Protocol error({callback.Method}): Target closed." + )); } + _callbacks.Clear(); Connection = null; - - foreach (var entry in _callbacks) - { - if (_callbacks.TryRemove(entry.Key, out _)) - { - entry.Value.TaskWrapper.TrySetException( - new TargetClosedException($"Protocol error({entry.Value.Method}): Target closed.")); - } - } } internal CDPSession CreateSession(TargetType targetType, string sessionId) { var session = new CDPSession(this, targetType, sessionId); - _sessions.TryAdd(sessionId, session); + _sessions[sessionId] = session; return session; } #endregion - #region IConnection ILoggerFactory IConnection.LoggerFactory => LoggerFactory; bool IConnection.IsClosed => IsClosed; diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs index 5622c5726..c807e1f13 100755 --- a/lib/PuppeteerSharp/Connection.cs +++ b/lib/PuppeteerSharp/Connection.cs @@ -1,6 +1,6 @@ using System; -using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using System.Net.WebSockets; using System.Text; using System.Threading; @@ -27,16 +27,22 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa WebSocket = ws; _logger = LoggerFactory.CreateLogger(); + _socketQueue = new TaskQueue(); + _responses = new Dictionary(); + _sessions = new Dictionary(); + _websocketReaderCancellationSource = new CancellationTokenSource(); Task.Factory.StartNew(GetResponseAsync); } #region Private Members private int _lastId; - private readonly ConcurrentDictionary _responses = new ConcurrentDictionary(); - private readonly ConcurrentDictionary _sessions = new ConcurrentDictionary(); - private readonly TaskQueue _socketQueue = new TaskQueue(); - private readonly CancellationTokenSource _websocketReaderCancellationSource = new CancellationTokenSource(); + private Dictionary _responses; + private Dictionary _sessions; + private TaskQueue _socketQueue; + private const string CloseMessage = "Browser.close"; + private bool _stopReading; + private CancellationTokenSource _websocketReaderCancellationSource; #endregion #region Properties @@ -44,17 +50,17 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa /// Gets the WebSocket URL. /// /// The URL. - public string Url { get; } + public string Url { get; private set; } /// /// Gets the sleep time when a message is received. /// /// The delay. - public int Delay { get; } + public int Delay { get; private set; } /// /// Gets the WebSocket. /// /// The web socket. - public WebSocket WebSocket { get; } + public WebSocket WebSocket { get; private set; } /// /// Occurs when the connection is closed. /// @@ -96,28 +102,23 @@ internal async Task SendAsync(string method, dynamic args = null) _logger.LogTrace("Send ► {Id} Method {Method} Params {@Params}", id, method, (object)args); - var callback = new MessageTask + var taskWrapper = new TaskCompletionSource(); + _responses[id] = new MessageTask { - TaskWrapper = new TaskCompletionSource(), + TaskWrapper = taskWrapper, Method = method }; - _responses.TryAdd(id, callback); - try - { - var encoded = Encoding.UTF8.GetBytes(message); - var buffer = new ArraySegment(encoded, 0, encoded.Length); - await _socketQueue.Enqueue(() => WebSocket.SendAsync(buffer, WebSocketMessageType.Text, true, default)).ConfigureAwait(false); - } - catch (Exception ex) + var encoded = Encoding.UTF8.GetBytes(message); + var buffer = new ArraySegment(encoded, 0, encoded.Length); + await _socketQueue.Enqueue(() => WebSocket.SendAsync(buffer, WebSocketMessageType.Text, true, default)).ConfigureAwait(false); + + if (method == CloseMessage) { - if (_responses.TryRemove(id, out _)) - { - callback.TaskWrapper.TrySetException(ex); - } + StopReading(); } - return await callback.TaskWrapper.Task.ConfigureAwait(false); + return await taskWrapper.Task.ConfigureAwait(false); } internal async Task SendAsync(string method, dynamic args = null) @@ -133,40 +134,41 @@ internal async Task CreateSessionAsync(TargetInfo targetInfo) targetId = targetInfo.TargetId }).ConfigureAwait(false)).sessionId; var session = new CDPSession(this, targetInfo.Type, sessionId); - _sessions.TryAdd(sessionId, session); + _sessions.Add(sessionId, session); return session; } #endregion - private void OnClose(Exception ex) + private void OnClose() { if (IsClosed) { return; } + IsClosed = true; _websocketReaderCancellationSource.Cancel(); Closed?.Invoke(this, new EventArgs()); - foreach (var entry in _sessions) + foreach (var session in _sessions.Values) { - if (_sessions.TryRemove(entry.Key, out _)) - { - entry.Value.OnClosed(); - } + session.OnClosed(); } - foreach (var entry in _responses) + foreach (var response in _responses.Values.Where(r => !r.TaskWrapper.Task.IsCompleted)) { - if (_responses.TryRemove(entry.Key, out _)) - { - entry.Value.TaskWrapper.TrySetException( - new TargetClosedException($"Protocol error({entry.Value.Method}): Target closed.", ex)); - } + response.TaskWrapper.SetException(new TargetClosedException( + $"Protocol error({response.Method}): Target closed." + )); } + + _responses.Clear(); + _sessions.Clear(); } + internal void StopReading() => _stopReading = true; + #region Private Methods /// @@ -178,86 +180,105 @@ private async Task GetResponseAsync() var buffer = new byte[2048]; //If it's not in the list we wait for it - while (!IsClosed) + while (true) { + if (IsClosed) + { + OnClose(); + return null; + } + var endOfMessage = false; - var response = new StringBuilder(); + var response = string.Empty; while (!endOfMessage) { - WebSocketReceiveResult result; + WebSocketReceiveResult result = null; try { result = await WebSocket.ReceiveAsync( new ArraySegment(buffer), _websocketReaderCancellationSource.Token).ConfigureAwait(false); } - catch (OperationCanceledException) + catch (Exception) when (_stopReading) { return null; } - catch (Exception ex) + catch (OperationCanceledException) { - OnClose(ex); return null; } + catch (Exception) + { + if (!IsClosed) + { + OnClose(); + return null; + } + } endOfMessage = result.EndOfMessage; if (result.MessageType == WebSocketMessageType.Text) { - response.Append(Encoding.UTF8.GetString(buffer, 0, result.Count)); + response += Encoding.UTF8.GetString(buffer, 0, result.Count); } else if (result.MessageType == WebSocketMessageType.Close) { - OnClose(null); + OnClose(); return null; } } - if (response.Length > 0) + if (!string.IsNullOrEmpty(response)) { if (Delay > 0) { await Task.Delay(Delay).ConfigureAwait(false); } - ProcessResponse(response.ToString()); + ProcessResponse(response); } } - - return null; } private void ProcessResponse(string response) { dynamic obj = JsonConvert.DeserializeObject(response); - var objAsJObject = (JObject)obj; + var objAsJObject = obj as JObject; _logger.LogTrace("◀ Receive {Message}", response); - var id = (int?)objAsJObject["id"]; - if (id.HasValue) + if (objAsJObject["id"] != null) { - if (_responses.TryRemove(id.Value, out var callback)) + var id = (int)objAsJObject["id"]; + + //If we get the object we are waiting for we return if + //if not we add this to the list, sooner or later some one will come for it + if (!_responses.ContainsKey(id)) { - callback.TaskWrapper.TrySetResult(obj.result); + _responses[id] = new MessageTask { TaskWrapper = new TaskCompletionSource() }; } + + _responses[id].TaskWrapper.SetResult(obj.result); } else { if (obj.method == "Target.receivedMessageFromTarget") { - if (_sessions.TryGetValue(objAsJObject["params"]["sessionId"].ToString(), out var session)) + var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); + if (session != null) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - if (_sessions.TryRemove(objAsJObject["params"]["sessionId"].ToString(), out var session) && !session.IsClosed) + var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); + if (!(session?.IsClosed ?? true)) { session.OnClosed(); + _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } else @@ -265,14 +286,12 @@ private void ProcessResponse(string response) MessageReceived?.Invoke(this, new MessageEventArgs { MessageID = obj.method, - MessageData = objAsJObject["params"] + MessageData = objAsJObject["params"] as dynamic }); } } } - #endregion - #region Static Methods /// @@ -303,7 +322,7 @@ internal static async Task Create(string url, IConnectionOptions con /// was occupying. public void Dispose() { - OnClose(new ObjectDisposedException($"Connection({Url})")); + OnClose(); WebSocket.Dispose(); } #endregion diff --git a/lib/PuppeteerSharp/TargetClosedException.cs b/lib/PuppeteerSharp/TargetClosedException.cs index 23a319c58..10811b0d8 100644 --- a/lib/PuppeteerSharp/TargetClosedException.cs +++ b/lib/PuppeteerSharp/TargetClosedException.cs @@ -1,6 +1,4 @@ -using System; - -namespace PuppeteerSharp +namespace PuppeteerSharp { /// /// Exception thrown by the when it detects that the target was closed. @@ -14,14 +12,5 @@ public class TargetClosedException : PuppeteerException public TargetClosedException(string message) : base(message) { } - - /// - /// Initializes a new instance of the class. - /// - /// Message. - /// Inner exception. - public TargetClosedException(string message, Exception innerException) : base(message, innerException) - { - } } } \ No newline at end of file From c5abbb3f7a57804be21072f89a1e6fc9c2c1648d Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Wed, 5 Sep 2018 16:20:31 +0200 Subject: [PATCH 8/9] Reduce chance of race conditions in Connection and CDPSession session and response message handling --- lib/PuppeteerSharp/Browser.cs | 1 - lib/PuppeteerSharp/CDPSession.cs | 43 +++++++++++------- lib/PuppeteerSharp/Connection.cs | 76 ++++++++++++-------------------- 3 files changed, 54 insertions(+), 66 deletions(-) diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index 9420b9ebb..c22b065ea 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -239,7 +239,6 @@ public async Task GetUserAgentAsync() private async Task CloseCoreAsync() { - Connection.StopReading(); try { try diff --git a/lib/PuppeteerSharp/CDPSession.cs b/lib/PuppeteerSharp/CDPSession.cs index 684c5f890..95d02c73d 100755 --- a/lib/PuppeteerSharp/CDPSession.cs +++ b/lib/PuppeteerSharp/CDPSession.cs @@ -1,10 +1,10 @@ using System; +using System.Linq; using System.Threading.Tasks; using Newtonsoft.Json; using System.Collections.Generic; using Newtonsoft.Json.Linq; using Microsoft.Extensions.Logging; -using PuppeteerSharp.Helpers; namespace PuppeteerSharp { @@ -126,7 +126,6 @@ internal async Task SendAsync(string method, bool rawContent, dynamic a Method = method, RawContent = rawContent }; - _callbacks[id] = callback; try @@ -163,18 +162,16 @@ public Task DetachAsync() internal void OnMessage(string message) { dynamic obj = JsonConvert.DeserializeObject(message); - var objAsJObject = obj as JObject; + var objAsJObject = (JObject)obj; _logger.LogTrace("◀ Receive {Message}", message); - if (objAsJObject["id"] != null && _callbacks.ContainsKey((int)obj.id)) + var id = (int?)objAsJObject["id"]; + if (id.HasValue && _callbacks.TryGetValue(id.Value, out var callback) && _callbacks.Remove(id.Value)) { - var callback = _callbacks[(int)obj.id]; - _callbacks.Remove((int)obj.id); - if (objAsJObject["error"] != null) { - callback.TaskWrapper.SetException(new MessageException( + callback.TaskWrapper.TrySetException(new MessageException( $"Protocol error ({ callback.Method }): {obj.error.message} {obj.error.data}" )); } @@ -182,11 +179,11 @@ internal void OnMessage(string message) { if (callback.RawContent) { - callback.TaskWrapper.SetResult(JsonConvert.SerializeObject(obj.result)); + callback.TaskWrapper.TrySetResult(JsonConvert.SerializeObject(obj.result)); } else { - callback.TaskWrapper.SetResult(obj.result); + callback.TaskWrapper.TrySetResult(obj.result); } } } @@ -201,19 +198,18 @@ internal void OnMessage(string message) } else if (obj.method == "Target.receivedMessageFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (session != null) + var sessionId = objAsJObject["params"]["sessionId"].ToString(); + if (_sessions.TryGetValue(sessionId, out var session)) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (!(session?.IsClosed ?? true)) + var sessionId = objAsJObject["params"]["sessionId"].ToString(); + if (_sessions.TryGetValue(sessionId, out var session) && _sessions.Remove(sessionId)) { session.OnClosed(); - _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } @@ -227,14 +223,26 @@ internal void OnMessage(string message) internal void OnClosed() { + if (IsClosed) + { + return; + } IsClosed = true; - foreach (var callback in _callbacks.Values) + + foreach (var session in _sessions.Values.ToArray()) { - callback.TaskWrapper.SetException(new TargetClosedException( + session.OnClosed(); + } + _sessions.Clear(); + + foreach (var callback in _callbacks.Values.ToArray()) + { + callback.TaskWrapper.TrySetException(new TargetClosedException( $"Protocol error({callback.Method}): Target closed." )); } _callbacks.Clear(); + Connection = null; } @@ -245,6 +253,7 @@ internal CDPSession CreateSession(TargetType targetType, string sessionId) return session; } #endregion + #region IConnection ILoggerFactory IConnection.LoggerFactory => LoggerFactory; bool IConnection.IsClosed => IsClosed; diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs index c807e1f13..3b7a0f418 100755 --- a/lib/PuppeteerSharp/Connection.cs +++ b/lib/PuppeteerSharp/Connection.cs @@ -37,12 +37,10 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa #region Private Members private int _lastId; - private Dictionary _responses; - private Dictionary _sessions; - private TaskQueue _socketQueue; - private const string CloseMessage = "Browser.close"; - private bool _stopReading; - private CancellationTokenSource _websocketReaderCancellationSource; + private readonly Dictionary _responses; + private readonly Dictionary _sessions; + private readonly TaskQueue _socketQueue; + private readonly CancellationTokenSource _websocketReaderCancellationSource; #endregion #region Properties @@ -50,17 +48,17 @@ internal Connection(string url, int delay, WebSocket ws, ILoggerFactory loggerFa /// Gets the WebSocket URL. /// /// The URL. - public string Url { get; private set; } + public string Url { get; } /// /// Gets the sleep time when a message is received. /// /// The delay. - public int Delay { get; private set; } + public int Delay { get; } /// /// Gets the WebSocket. /// /// The web socket. - public WebSocket WebSocket { get; private set; } + public WebSocket WebSocket { get; } /// /// Occurs when the connection is closed. /// @@ -102,23 +100,18 @@ internal async Task SendAsync(string method, dynamic args = null) _logger.LogTrace("Send ► {Id} Method {Method} Params {@Params}", id, method, (object)args); - var taskWrapper = new TaskCompletionSource(); - _responses[id] = new MessageTask + var callback = new MessageTask { - TaskWrapper = taskWrapper, + TaskWrapper = new TaskCompletionSource(), Method = method }; + _responses[id] = callback; var encoded = Encoding.UTF8.GetBytes(message); var buffer = new ArraySegment(encoded, 0, encoded.Length); await _socketQueue.Enqueue(() => WebSocket.SendAsync(buffer, WebSocketMessageType.Text, true, default)).ConfigureAwait(false); - if (method == CloseMessage) - { - StopReading(); - } - - return await taskWrapper.Task.ConfigureAwait(false); + return await callback.TaskWrapper.Task.ConfigureAwait(false); } internal async Task SendAsync(string method, dynamic args = null) @@ -145,30 +138,26 @@ private void OnClose() { return; } - IsClosed = true; _websocketReaderCancellationSource.Cancel(); Closed?.Invoke(this, new EventArgs()); - foreach (var session in _sessions.Values) + foreach (var session in _sessions.Values.ToArray()) { session.OnClosed(); } + _sessions.Clear(); - foreach (var response in _responses.Values.Where(r => !r.TaskWrapper.Task.IsCompleted)) + foreach (var response in _responses.Values.ToArray()) { - response.TaskWrapper.SetException(new TargetClosedException( + response.TaskWrapper.TrySetException(new TargetClosedException( $"Protocol error({response.Method}): Target closed." )); } - _responses.Clear(); - _sessions.Clear(); } - internal void StopReading() => _stopReading = true; - #region Private Methods /// @@ -193,28 +182,21 @@ private async Task GetResponseAsync() while (!endOfMessage) { - WebSocketReceiveResult result = null; + WebSocketReceiveResult result; try { result = await WebSocket.ReceiveAsync( new ArraySegment(buffer), _websocketReaderCancellationSource.Token).ConfigureAwait(false); } - catch (Exception) when (_stopReading) - { - return null; - } catch (OperationCanceledException) { return null; } catch (Exception) { - if (!IsClosed) - { - OnClose(); - return null; - } + OnClose(); + return null; } endOfMessage = result.EndOfMessage; @@ -245,7 +227,7 @@ private async Task GetResponseAsync() private void ProcessResponse(string response) { dynamic obj = JsonConvert.DeserializeObject(response); - var objAsJObject = obj as JObject; + var objAsJObject = (JObject)obj; _logger.LogTrace("◀ Receive {Message}", response); @@ -255,30 +237,27 @@ private void ProcessResponse(string response) //If we get the object we are waiting for we return if //if not we add this to the list, sooner or later some one will come for it - if (!_responses.ContainsKey(id)) + if (_responses.TryGetValue(id, out var callback)) { - _responses[id] = new MessageTask { TaskWrapper = new TaskCompletionSource() }; + callback.TaskWrapper.TrySetResult(obj.result); } - - _responses[id].TaskWrapper.SetResult(obj.result); } else { if (obj.method == "Target.receivedMessageFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (session != null) + var sessionId = objAsJObject["params"]["sessionId"].ToString(); + if (_sessions.TryGetValue(sessionId, out var session)) { session.OnMessage(objAsJObject["params"]["message"].ToString()); } } else if (obj.method == "Target.detachedFromTarget") { - var session = _sessions.GetValueOrDefault(objAsJObject["params"]["sessionId"].ToString()); - if (!(session?.IsClosed ?? true)) + var sessionId = objAsJObject["params"]["sessionId"].ToString(); + if (_sessions.TryGetValue(sessionId, out var session) && _sessions.Remove(sessionId) && !session.IsClosed) { session.OnClosed(); - _sessions.Remove(objAsJObject["params"]["sessionId"].ToString()); } } else @@ -286,12 +265,13 @@ private void ProcessResponse(string response) MessageReceived?.Invoke(this, new MessageEventArgs { MessageID = obj.method, - MessageData = objAsJObject["params"] as dynamic + MessageData = objAsJObject["params"] }); } } } #endregion + #region Static Methods /// @@ -313,7 +293,7 @@ internal static async Task Create(string url, IConnectionOptions con /// /// Releases all resource used by the object. - /// It will raise the event and call . + /// It will raise the event and dispose . /// /// Call when you are finished using the . The /// method leaves the in an unusable state. From 0e3ae4b487a31c0318e2a97105622a93d2893664 Mon Sep 17 00:00:00 2001 From: Gerke Geurts Date: Wed, 5 Sep 2018 21:21:57 +0200 Subject: [PATCH 9/9] Introduce Browser.CloseTimeout constant --- lib/PuppeteerSharp/Browser.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/PuppeteerSharp/Browser.cs b/lib/PuppeteerSharp/Browser.cs index c22b065ea..76729ce90 100644 --- a/lib/PuppeteerSharp/Browser.cs +++ b/lib/PuppeteerSharp/Browser.cs @@ -35,6 +35,11 @@ namespace PuppeteerSharp /// public class Browser : IDisposable { + /// + /// Time in milliseconds for chromium process to exit gracefully. + /// + private const int CloseTimeout = 5000; + /// /// Initializes a new instance of the class. /// @@ -251,7 +256,7 @@ private async Task CloseCoreAsync() { // Notify chromium process that exit is expected, but should be enforced if it // doesn't occur withing the close timeout. - var closeTimeout = TimeSpan.FromMilliseconds(5000); + var closeTimeout = TimeSpan.FromMilliseconds(CloseTimeout); await _chromiumProcess.EnsureExitAsync(closeTimeout).ConfigureAwait(false); }