-
-
Notifications
You must be signed in to change notification settings - Fork 443
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Addresses deadlock, thread-safety in mixed sync/async apps (#1549)
* Fixed deadlock, thread-safety in mixed sync/async apps where if a continuation from CDP.SendAsync in returns calls CDP.SendAsync but uses .GetResult(), then deadlock. CDP event messages are still delivered like before, since their order must be preserved. There's potential for deadlock here, but this is still the caller's responsibility to handle events correctly. * Using expr body like before * Reverted queue change, added test * Fixed race in test * Reverted if branch change * Reverted change to test * Reverted dropped unused using * Added lost BOM from my previous commits to this * Refactored new SendAsync response handling into its own place * Possible solution for ensuring async tasks are done * Addressed false positive disposal warning * Simplified new task queue Important part is really only DrainAsync * Added EnqueueSendAsyncResponses option similar to EnqueueTransportMessages, to at least provide a way to turn this new async behavior off * Implemented handling for new EnqueueSendAsyncResponses option * Temp disabling EnqueueSendAsyncResponses for test run * Reverted temp change * Temp reverting CSSCoverage changes which could be causing tests to hang * Reverted previous commit * Ensuring canceled task is handled * Fixed null ref founding during local test run * Attempting to roll back regression causing deadlock * Reverted previous, only logging unhandled exception * Reverted cancel token param * Using alternate dispose impl * Reverted accidental whitespace and BOM changes * Renames, code cleanup, defaulting new async option to false * Consolidated into single ConcurrentDictionary * Renamed test var to match renamed option name * Modified test to more closely match what was reported in #1354 * Added ENQUEUE_ASYNC_MESSAGES variation * Second attempt on appveyor matrix * Ignoring test unless required option is set * Update lib/PuppeteerSharp/CDPSession.cs * Update appveyor.yml Co-authored-by: Jeff Peirson <Jeff.Peirson@a2ius.com> Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
- Loading branch information
3 people
authored
Nov 8, 2020
1 parent
616425c
commit d5044c6
Showing
12 changed files
with
317 additions
and
54 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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("<html><body>REPLACED</body></html>").ConfigureAwait(false); | ||
|
|
||
| // Deep inside an existing mostly sync app... | ||
| var content = page.GetContentAsync().ConfigureAwait(false).GetAwaiter().GetResult(); | ||
| Assert.Contains("REPLACE", content); | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| { | ||
| /// <summary> | ||
| /// Provides an async queue for responses for <see cref="CDPSession.SendAsync"/>, so that responses can be handled | ||
| /// async without risk callers causing a deadlock. | ||
| /// </summary> | ||
| internal class AsyncMessageQueue : IDisposable | ||
| { | ||
| private bool _disposed; | ||
| private readonly List<MessageTask> _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<MessageTask>(); | ||
| } | ||
|
|
||
| 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); | ||
| } | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.