diff --git a/appveyor.yml b/appveyor.yml index 7d369b0b5..eeedbce47 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -10,6 +10,14 @@ environment: matrix: - framework: net48 - framework: netcoreapp2.2 + - framework: net48 + ENQUEUE_ASYNC_MESSAGES: true + - framework: netcoreapp2.2 + ENQUEUE_ASYNC_MESSAGES: true + - framework: net48 + ENQUEUE_ASYNC_MESSAGES: false + - framework: netcoreapp2.2 + ENQUEUE_ASYNC_MESSAGES: false install: - ps: >- if($env:APPVEYOR_REPO_TAG -eq 'True' -And $env:framework -eq 'netcoreapp2.0') { @@ -39,4 +47,4 @@ on_success: c:\projects\puppeteer-sharp\appveyor\GenerateDocs.ps1 cache: - - $HOME/.nuget/packages \ No newline at end of file + - $HOME/.nuget/packages diff --git a/lib/PuppeteerSharp.Tests/Issues/Issue1354.cs b/lib/PuppeteerSharp.Tests/Issues/Issue1354.cs new file mode 100644 index 000000000..ee23f98f5 --- /dev/null +++ b/lib/PuppeteerSharp.Tests/Issues/Issue1354.cs @@ -0,0 +1,53 @@ +using System.Threading.Tasks; +using Xunit; +using Xunit.Abstractions; + +namespace PuppeteerSharp.Tests.Issues +{ + [Collection(TestConstants.TestFixtureCollectionName)] + public class Issue1354 : PuppeteerPageBaseTest + { + public Issue1354(ITestOutputHelper output) : base(output) + { + } + + [Fact(Timeout = 30000)] + public async Task ShouldAllowSyncClose() + { + var options = TestConstants.DefaultBrowserOptions(); + if (!options.EnqueueAsyncMessages) + { + // This test won't pass unless this option is set to true. + return; + } + + using (var browser = await Puppeteer.LaunchAsync(options).ConfigureAwait(false)) + { + // In issue #1354, this line hangs forever + browser.CloseAsync().Wait(); + } + } + + [Fact(Timeout = 30000)] + public async Task ShouldAllowSyncPageMethod() + { + var options = TestConstants.DefaultBrowserOptions(); + if (!options.EnqueueAsyncMessages) + { + return; + } + + using (var browser = await Puppeteer.LaunchAsync(options)) + { + // Async low-level use + await using var page = await browser.NewPageAsync().ConfigureAwait(false); + await page.GoToAsync("http://ipecho.net/plain", WaitUntilNavigation.DOMContentLoaded).ConfigureAwait(false); + await page.SetContentAsync("REPLACED").ConfigureAwait(false); + + // Deep inside an existing mostly sync app... + var content = page.GetContentAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + Assert.Contains("REPLACE", content); + } + } + } +} diff --git a/lib/PuppeteerSharp.Tests/TestConstants.cs b/lib/PuppeteerSharp.Tests/TestConstants.cs index 7aa8fc395..2b5fda56a 100644 --- a/lib/PuppeteerSharp.Tests/TestConstants.cs +++ b/lib/PuppeteerSharp.Tests/TestConstants.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -43,6 +43,7 @@ public static class TestConstants { SlowMo = Convert.ToInt32(Environment.GetEnvironmentVariable("SLOW_MO")), Headless = Convert.ToBoolean(Environment.GetEnvironmentVariable("HEADLESS") ?? "true"), + EnqueueAsyncMessages = Convert.ToBoolean(Environment.GetEnvironmentVariable("ENQUEUE_ASYNC_MESSAGES") ?? "false"), Timeout = 0, LogProcess = true, #if NETCOREAPP @@ -70,4 +71,4 @@ public static void SetupLogging(ITestOutputHelper output) } } } -} \ No newline at end of file +} diff --git a/lib/PuppeteerSharp/CDPSession.cs b/lib/PuppeteerSharp/CDPSession.cs index 0c344db70..e80de6203 100644 --- a/lib/PuppeteerSharp/CDPSession.cs +++ b/lib/PuppeteerSharp/CDPSession.cs @@ -1,5 +1,5 @@ using System; -using System.Collections.Concurrent; +using System.Collections.Concurrent; using System.Linq; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -118,7 +118,7 @@ public async Task SendAsync(string method, object args = null) /// If false the task will be considered complete after sending the message to Chromium. /// /// The task. - /// + /// public async Task SendAsync(string method, object args = null, bool waitForCallback = true) { if (Connection == null) @@ -186,14 +186,7 @@ internal void OnMessage(ConnectionResponse obj) if (id.HasValue && _callbacks.TryRemove(id.Value, out var callback)) { - if (obj.Error != null) - { - callback.TaskWrapper.TrySetException(new MessageException(callback, obj.Error)); - } - else - { - callback.TaskWrapper.TrySetResult(obj.Result); - } + Connection.MessageQueue.Enqueue(callback, obj); } else { @@ -232,4 +225,4 @@ internal void Close(string closeReason) #endregion } -} \ No newline at end of file +} diff --git a/lib/PuppeteerSharp/ConnectOptions.cs b/lib/PuppeteerSharp/ConnectOptions.cs index 7c682f6a6..a7bd418f5 100644 --- a/lib/PuppeteerSharp/ConnectOptions.cs +++ b/lib/PuppeteerSharp/ConnectOptions.cs @@ -73,5 +73,17 @@ public class ConnectOptions : IBrowserOptions, IConnectionOptions /// Setting this to false proved to work in .NET Core but it tends to fail on .NET Framework. /// public bool EnqueueTransportMessages { get; set; } = true; + + /// + /// Affects how responses to are returned to the caller. If true (default), the + /// response is delivered to the caller on its own thread; otherwise, the response is delivered the same way + /// events are raised. + /// + /// + /// This should normally be set to true to support applications that aren't async "all the way up"; i.e., the application + /// has legacy code that is not async which makes calls into PuppeteerSharp. If you experience issues, or your application is not mixed sync/async use, you + /// can set this to false (default). + /// + public bool EnqueueAsyncMessages { get; set; } } } diff --git a/lib/PuppeteerSharp/Connection.cs b/lib/PuppeteerSharp/Connection.cs index 66bb38e79..4650c266a 100644 --- a/lib/PuppeteerSharp/Connection.cs +++ b/lib/PuppeteerSharp/Connection.cs @@ -21,7 +21,7 @@ public class Connection : IDisposable private readonly ILogger _logger; private TaskQueue _callbackQueue = new TaskQueue(); - internal Connection(string url, int delay, IConnectionTransport transport, ILoggerFactory loggerFactory = null) + internal Connection(string url, int delay, bool enqueueAsyncMessages, IConnectionTransport transport, ILoggerFactory loggerFactory = null) { LoggerFactory = loggerFactory ?? new LoggerFactory(); Url = url; @@ -34,6 +34,7 @@ internal Connection(string url, int delay, IConnectionTransport transport, ILogg Transport.Closed += Transport_Closed; _callbacks = new ConcurrentDictionary(); _sessions = new ConcurrentDictionary(); + MessageQueue = new AsyncMessageQueue(enqueueAsyncMessages, _logger); _asyncSessions = new AsyncDictionaryHelper(_sessions, "Session {0} not found"); } @@ -85,6 +86,8 @@ internal Connection(string url, int delay, IConnectionTransport transport, ILogg /// The logger factory. public ILoggerFactory LoggerFactory { get; } + internal AsyncMessageQueue MessageQueue { get; } + #endregion #region Public Methods @@ -176,6 +179,7 @@ internal void Close(string closeReason) } _callbacks.Clear(); + MessageQueue.Dispose(); } internal static Connection FromSession(CDPSession session) => session.Connection; @@ -245,7 +249,7 @@ private void ProcessIncomingMessage(ConnectionResponse obj) if (!string.IsNullOrEmpty(obj.SessionId)) { var session = GetSession(obj.SessionId); - session.OnMessage(obj); + session?.OnMessage(obj); } else if (obj.Id.HasValue) { @@ -253,14 +257,7 @@ private void ProcessIncomingMessage(ConnectionResponse obj) // if not we add this to the list, sooner or later some one will come for it if (_callbacks.TryRemove(obj.Id.Value, out var callback)) { - if (obj.Error != null) - { - callback.TaskWrapper.TrySetException(new MessageException(callback, obj.Error)); - } - else - { - callback.TaskWrapper.TrySetResult(obj.Result); - } + MessageQueue.Enqueue(callback, obj); } } else @@ -296,7 +293,7 @@ internal static async Task Create(string url, IConnectionOptions con transport = await transportFactory(new Uri(url), connectionOptions, cancellationToken).ConfigureAwait(false); } - return new Connection(url, connectionOptions.SlowMo, transport, loggerFactory); + return new Connection(url, connectionOptions.SlowMo, connectionOptions.EnqueueAsyncMessages, transport, loggerFactory); } /// diff --git a/lib/PuppeteerSharp/FrameManager.cs b/lib/PuppeteerSharp/FrameManager.cs index 5c8b89c7f..454d76758 100644 --- a/lib/PuppeteerSharp/FrameManager.cs +++ b/lib/PuppeteerSharp/FrameManager.cs @@ -13,7 +13,7 @@ namespace PuppeteerSharp { internal class FrameManager { - private readonly Dictionary _contextIdToContext; + private readonly ConcurrentDictionary _contextIdToContext; private bool _ensureNewDocumentNavigation; private readonly ILogger _logger; private readonly ConcurrentDictionary _frames; @@ -27,7 +27,7 @@ private FrameManager(CDPSession client, Page page, bool ignoreHTTPSErrors, Timeo Client = client; Page = page; _frames = new ConcurrentDictionary(); - _contextIdToContext = new Dictionary(); + _contextIdToContext = new ConcurrentDictionary(); _logger = Client.Connection.LoggerFactory.CreateLogger(); NetworkManager = new NetworkManager(client, ignoreHTTPSErrors, this); TimeoutSettings = timeoutSettings; @@ -240,24 +240,21 @@ private void OnExecutionContextsCleared() { while (_contextIdToContext.Count > 0) { - var contextItem = _contextIdToContext.ElementAt(0); - _contextIdToContext.Remove(contextItem.Key); - - if (contextItem.Value.World != null) + int key0 = _contextIdToContext.Keys.ElementAtOrDefault(0); + if (_contextIdToContext.TryRemove(key0, out var context)) { - contextItem.Value.World.SetContext(null); + if (context.World != null) + { + context.World.SetContext(null); + } } } } private void OnExecutionContextDestroyed(int executionContextId) { - _contextIdToContext.TryGetValue(executionContextId, out var context); - - if (context != null) + if (_contextIdToContext.TryRemove(executionContextId, out var context)) { - _contextIdToContext.Remove(executionContextId); - if (context.World != null) { context.World.SetContext(null); diff --git a/lib/PuppeteerSharp/Helpers/AsyncMessageQueue.cs b/lib/PuppeteerSharp/Helpers/AsyncMessageQueue.cs new file mode 100644 index 000000000..539112d32 --- /dev/null +++ b/lib/PuppeteerSharp/Helpers/AsyncMessageQueue.cs @@ -0,0 +1,102 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using PuppeteerSharp.Messaging; + +namespace PuppeteerSharp.Helpers +{ + /// + /// Provides an async queue for responses for , so that responses can be handled + /// async without risk callers causing a deadlock. + /// + internal class AsyncMessageQueue : IDisposable + { + private bool _disposed; + private readonly List _pendingTasks; + private readonly bool _enqueueAsyncMessages; + private readonly ILogger _logger; + + public AsyncMessageQueue(bool enqueueAsyncMessages, ILogger logger = null) + { + _enqueueAsyncMessages = enqueueAsyncMessages; + _logger = logger ?? NullLogger.Instance; + _pendingTasks = new List(); + } + + public void Enqueue(MessageTask callback, ConnectionResponse obj) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + + if (!_enqueueAsyncMessages) + { + HandleAsyncMessage(callback, obj); + return; + } + + // Keep a ref to this task until it completes. If it can't finish by the time we dispose this queue, + // then we'll find it and cancel it. + lock (_pendingTasks) + { + _pendingTasks.Add(callback); + } + + var task = Task.Run(() => HandleAsyncMessage(callback, obj)); + + // Unhandled error handler + task.ContinueWith(t => + { + _logger.LogError(t.Exception, "Failed to complete async handling of SendAsync for {callback}", callback.Method); + callback.TaskWrapper.TrySetException(t.Exception!); // t.Exception is available since this runs only on faulted + }, TaskContinuationOptions.OnlyOnFaulted); + + // Always remove from the queue when done, regardless of outcome. + task.ContinueWith(_ => + { + lock (_pendingTasks) + { + _pendingTasks.Remove(callback); + } + }); + } + + public void Dispose() + { + if (_disposed) + { + return; + } + + // Ensure all tasks are finished since we're disposing now. Any pending tasks will be canceled. + MessageTask[] pendingTasks; + lock (_pendingTasks) + { + pendingTasks = _pendingTasks.ToArray(); + _pendingTasks.Clear(); + } + + foreach (var pendingTask in pendingTasks) + { + pendingTask.TaskWrapper.TrySetCanceled(); + } + + _disposed = true; + } + + private static void HandleAsyncMessage(MessageTask callback, ConnectionResponse obj) + { + if (obj.Error != null) + { + callback.TaskWrapper.TrySetException(new MessageException(callback, obj.Error)); + } + else + { + callback.TaskWrapper.TrySetResult(obj.Result); + } + } + } +} diff --git a/lib/PuppeteerSharp/Helpers/DeferredTaskQueue.cs b/lib/PuppeteerSharp/Helpers/DeferredTaskQueue.cs new file mode 100644 index 000000000..933ab46bf --- /dev/null +++ b/lib/PuppeteerSharp/Helpers/DeferredTaskQueue.cs @@ -0,0 +1,78 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +namespace PuppeteerSharp.Helpers +{ + /// + /// An async queue that accepts a task but defers its execution to be handled by a consumer queue. + /// + internal class DeferredTaskQueue + { + private readonly List _pendingTasks; + + public DeferredTaskQueue() + { + _pendingTasks = new List(); + } + + public async Task Enqueue(Func taskGenerator) + { + var task = taskGenerator(); + if (task.IsCompleted) + { + // Don't need to do anything. + return; + } + + try + { + lock (_pendingTasks) + { + _pendingTasks.Add(task); + } + + await task.ConfigureAwait(false); + } + finally + { + lock (_pendingTasks) + { + _pendingTasks.Remove(task); + } + } + } + + public async Task DrainAsync(CancellationToken cancellationToken = default) + { + while (!cancellationToken.IsCancellationRequested) + { + Task task; + lock (_pendingTasks) + { + if (_pendingTasks.Count == 0) + { + break; + } + + task = _pendingTasks[0]; + } + + try + { + await task.ConfigureAwait(false); + } + finally + { + lock (_pendingTasks) + { + _pendingTasks.Remove(task); + } + } + } + + cancellationToken.ThrowIfCancellationRequested(); + } + } +} diff --git a/lib/PuppeteerSharp/IConnectionOptions.cs b/lib/PuppeteerSharp/IConnectionOptions.cs index ac8c5b48d..87309d0d7 100644 --- a/lib/PuppeteerSharp/IConnectionOptions.cs +++ b/lib/PuppeteerSharp/IConnectionOptions.cs @@ -41,5 +41,10 @@ public interface IConnectionOptions /// If not is set this will be use to determine is the default will enqueue messages. /// bool EnqueueTransportMessages { get; set; } + + /// + /// Affects how responses to are returned to the caller. + /// + bool EnqueueAsyncMessages { get; set; } } -} \ No newline at end of file +} diff --git a/lib/PuppeteerSharp/LaunchOptions.cs b/lib/PuppeteerSharp/LaunchOptions.cs index 4d588c150..0ae3f1bbc 100644 --- a/lib/PuppeteerSharp/LaunchOptions.cs +++ b/lib/PuppeteerSharp/LaunchOptions.cs @@ -159,5 +159,17 @@ public string[] IgnoredDefaultArgs /// Setting this to false proved to work in .NET Core but it tends to fail on .NET Framework. /// public bool EnqueueTransportMessages { get; set; } = true; + + /// + /// Affects how responses to are returned to the caller. If true (default), the + /// response is delivered to the caller on its own thread; otherwise, the response is delivered the same way + /// events are raised. + /// + /// + /// This should normally be set to true to support applications that aren't async "all the way up"; i.e., the application + /// has legacy code that is not async which makes calls into PuppeteerSharp. If you experience issues, or your application is not mixed sync/async use, you + /// can set this to false (default). + /// + public bool EnqueueAsyncMessages { get; set; } } -} \ No newline at end of file +} diff --git a/lib/PuppeteerSharp/PageCoverage/CSSCoverage.cs b/lib/PuppeteerSharp/PageCoverage/CSSCoverage.cs index 0e654fe61..a56fc723c 100755 --- a/lib/PuppeteerSharp/PageCoverage/CSSCoverage.cs +++ b/lib/PuppeteerSharp/PageCoverage/CSSCoverage.cs @@ -1,7 +1,9 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using PuppeteerSharp.Helpers; using PuppeteerSharp.Helpers.Json; using PuppeteerSharp.Messaging; @@ -10,8 +12,8 @@ namespace PuppeteerSharp.PageCoverage internal class CSSCoverage { private readonly CDPSession _client; - private readonly Dictionary _stylesheetURLs; - private readonly Dictionary _stylesheetSources; + private readonly ConcurrentDictionary _stylesheets; + private readonly DeferredTaskQueue _callbackQueue; private readonly ILogger _logger; private bool _enabled; @@ -21,9 +23,9 @@ public CSSCoverage(CDPSession client) { _client = client; _enabled = false; - _stylesheetURLs = new Dictionary(); - _stylesheetSources = new Dictionary(); + _stylesheets = new ConcurrentDictionary(); _logger = _client.Connection.LoggerFactory.CreateLogger(); + _callbackQueue = new DeferredTaskQueue(); _resetOnNavigation = false; } @@ -37,8 +39,7 @@ internal Task StartAsync(CoverageStartOptions options) _resetOnNavigation = options.ResetOnNavigation; _enabled = true; - _stylesheetURLs.Clear(); - _stylesheetSources.Clear(); + _stylesheets.Clear(); _client.MessageReceived += Client_MessageReceived; @@ -57,10 +58,15 @@ internal async Task StopAsync() _enabled = false; var trackingResponse = await _client.SendAsync("CSS.stopRuleUsageTracking").ConfigureAwait(false); + + // Wait until we've stopped CSS tracking before stopping listening for messages and finishing up, so that + // any pending OnStyleSheetAddedAsync tasks can collect the remaining style sheet coverage. + _client.MessageReceived -= Client_MessageReceived; + await _callbackQueue.DrainAsync().ConfigureAwait(false); + await Task.WhenAll( _client.SendAsync("CSS.disable"), _client.SendAsync("DOM.disable")).ConfigureAwait(false); - _client.MessageReceived -= Client_MessageReceived; var styleSheetIdToCoverage = new Dictionary>(); foreach (var entry in trackingResponse.RuleUsage) @@ -80,10 +86,11 @@ await Task.WhenAll( } var coverage = new List(); - foreach (var styleSheetId in _stylesheetURLs.Keys) + foreach (var kv in _stylesheets.ToArray()) { - var url = _stylesheetURLs[styleSheetId]; - var text = _stylesheetSources[styleSheetId]; + var styleSheetId = kv.Key; + var url = kv.Value.Url; + var text = kv.Value.Source; styleSheetIdToCoverage.TryGetValue(styleSheetId, out var responseRanges); var ranges = Coverage.ConvertToDisjointRanges(responseRanges ?? new List()); coverage.Add(new CoverageEntry @@ -103,7 +110,7 @@ private async void Client_MessageReceived(object sender, MessageEventArgs e) switch (e.MessageID) { case "CSS.styleSheetAdded": - await OnStyleSheetAddedAsync(e.MessageData.ToObject(true)).ConfigureAwait(false); + await _callbackQueue.Enqueue(() => OnStyleSheetAddedAsync(e.MessageData.ToObject(true))).ConfigureAwait(false); break; case "Runtime.executionContextsCleared": OnExecutionContextsCleared(); @@ -132,8 +139,7 @@ private async Task OnStyleSheetAddedAsync(CSSStyleSheetAddedResponse styleSheetA StyleSheetId = styleSheetAddedResponse.Header.StyleSheetId }).ConfigureAwait(false); - _stylesheetURLs.Add(styleSheetAddedResponse.Header.StyleSheetId, styleSheetAddedResponse.Header.SourceURL); - _stylesheetSources.Add(styleSheetAddedResponse.Header.StyleSheetId, response.Text); + _stylesheets.TryAdd(styleSheetAddedResponse.Header.StyleSheetId, (styleSheetAddedResponse.Header.SourceURL, response.Text)); } catch (Exception ex) { @@ -148,8 +154,7 @@ private void OnExecutionContextsCleared() return; } - _stylesheetURLs.Clear(); - _stylesheetSources.Clear(); + _stylesheets.Clear(); } } }