Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fix race condition that caused LaunchAsync to never resolve for chrome #2214

Merged
merged 15 commits into from Jun 13, 2023
Merged

Fix race condition that caused LaunchAsync to never resolve for chrome #2214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ca95043
Fix https://github.com/hardkoded/puppeteer-sharp/issues/2202
leonardo-fernandes May 26, 2023
5a5391b
Change the await to only apply to attachedToTarget, leaving other mes…
leonardo-fernandes May 27, 2023
f58262f
Remove the await for OnAttachedToTarget call, and also included a mis…
leonardo-fernandes May 30, 2023
aabb86e
* Fixed a race condition if a message is received before the Browser.…
leonardo-fernandes Jun 1, 2023
deb351d
Previous commit introduced a new race condition. It was possible that…
leonardo-fernandes Jun 1, 2023
84c3dd5
It was possible for the TargetManager initialization to finish withou…
leonardo-fernandes Jun 2, 2023
dda9812
PR review
leonardo-fernandes Jun 2, 2023
91c5e2b
Rolling back Target.setDiscoverTargets to be sent from the constructor
leonardo-fernandes Jun 10, 2023
7e65695
Handle exceptions in OnAttachedToTarget
leonardo-fernandes Jun 10, 2023
0d3a600
OnAttachedToTarget should be executed synchronously if possible, so t…
leonardo-fernandes Jun 10, 2023
16b9283
Fix PuppeteerConnectTests.ShouldSupportTargetFilter.
leonardo-fernandes Jun 12, 2023
d7d6617
Fix OOPIFTests.ShouldDetectExistingOopifsWhenPuppeteerConnectsToAnExi…
leonardo-fernandes Jun 12, 2023
e8199a6
Revert unintentional line ending changes.
leonardo-fernandes Jun 12, 2023
f22b62a
Merge branch 'hardkoded:master' into master
leonardo-fernandes Jun 12, 2023
1bbb87f
Use the launcher timeout when awaiting for `_targetDiscoveryCompletio…
leonardo-fernandes Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/PuppeteerSharp.Tests/LauncherTests/PuppeteerConnectTests.cs
View file
Open in desktop
Expand Up @@ -91,7 +91,7 @@ await using (var page = await browser.NewPageAsync())
[SkipBrowserFact(skipFirefox: true)]
public async Task ShouldSupportTargetFilter()
{
await using (var originalBrowser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions()))
await using (var originalBrowser = await Puppeteer.LaunchAsync(TestConstants.DefaultBrowserOptions(), TestConstants.LoggerFactory))
{
var page1 = await originalBrowser.NewPageAsync();
await page1.GoToAsync(TestConstants.EmptyPage);
Expand All @@ -102,7 +102,7 @@ await using (var originalBrowser = await Puppeteer.LaunchAsync(TestConstants.Def
var browser = await Puppeteer.ConnectAsync(new ConnectOptions {
BrowserWSEndpoint = originalBrowser.WebSocketEndpoint,
TargetFilter = (TargetInfo targetInfo) => !targetInfo.Url.Contains("should-be-ignored"),
});
}, TestConstants.LoggerFactory);

var pages = await browser.PagesAsync();

Expand Down
3 changes: 2 additions & 1 deletion lib/PuppeteerSharp.Tests/OOPIFTests/OOPIFTests.cs
View file
Open in desktop
Expand Up @@ -334,11 +334,12 @@ public async Task ShouldDetectExistingOopifsWhenPuppeteerConnectsToAnExistingPag
Assert.Equal(2, Page.Frames.Length);

var browserURL = $"http://127.0.0.1:{_port}";
using var browser1 = await Puppeteer.ConnectAsync(new (){ BrowserURL = browserURL });
var browser1 = await Puppeteer.ConnectAsync(new (){ BrowserURL = browserURL }, TestConstants.LoggerFactory);
var target = await browser1.WaitForTargetAsync((target) =>
target.Url.EndsWith("dynamic-oopif.html")
).WithTimeout();
await target.PageAsync();
browser1.Disconnect();
}

[PuppeteerTest("oopif.spec.ts", "OOPIF", "should support lazy OOP frames")]
Expand Down
33 changes: 33 additions & 0 deletions lib/PuppeteerSharp.Tests/UtilitiesTests/TaskQueueTests.cs
View file
Open in desktop
Expand Up @@ -49,6 +49,39 @@ public async Task ShouldNotThrowWhenDisposingMultipleTimesAsync()
await taskQueue.DisposeAsync().ConfigureAwait(false);
}

[Fact]
public async Task CanDisposeWhileSemaphoreIsHeld()
{
var taskQueue = new TaskQueue();

await taskQueue.Enqueue(() =>
{
taskQueue.Dispose();
return Task.CompletedTask;
});

var semaphore = GetSemaphore(taskQueue);
Assert.Throws<ObjectDisposedException>(() => semaphore.AvailableWaitHandle);

taskQueue.Dispose();
}

[Fact]
public async Task CanDisposeWhileSemaphoreIsHeldAsync()
{
var taskQueue = new TaskQueue();

await taskQueue.Enqueue(async () =>
{
await taskQueue.DisposeAsync();
});

var semaphore = GetSemaphore(taskQueue);
Assert.Throws<ObjectDisposedException>(() => semaphore.AvailableWaitHandle);

await taskQueue.DisposeAsync();
}

private static SemaphoreSlim GetSemaphore(TaskQueue queue) =>
(SemaphoreSlim)typeof(TaskQueue).GetField("_semaphore", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(queue);
}
Expand Down
23 changes: 8 additions & 15 deletions lib/PuppeteerSharp/Browser.cs
View file
Open in desktop
Expand Up @@ -73,7 +73,8 @@ public class Browser : IBrowser
TargetManager = new ChromeTargetManager(
connection,
CreateTarget,
_targetFilterCallback);
_targetFilterCallback,
launcher?.Options?.Timeout ?? Puppeteer.DefaultTimeout);
}
}

Expand Down Expand Up @@ -201,12 +202,6 @@ public async Task<ITarget> WaitForTargetAsync(Func<ITarget, bool> predicate, Wai
}

var timeout = options?.Timeout ?? DefaultWaitForTimeout;
var existingTarget = Targets().FirstOrDefault(predicate);
if (existingTarget != null)
{
return existingTarget;
}

var targetCompletionSource = new TaskCompletionSource<ITarget>(TaskCreationOptions.RunContinuationsAsynchronously);

void TargetHandler(object sender, TargetChangedArgs e)
Expand All @@ -219,17 +214,15 @@ void TargetHandler(object sender, TargetChangedArgs e)

try
{
foreach (var target in Targets())
{
if (predicate(target))
{
return target;
}
}

TargetCreated += TargetHandler;
TargetChanged += TargetHandler;

var existingTarget = Targets().FirstOrDefault(predicate);
if (existingTarget != null)
{
return existingTarget;
}
leonardo-fernandes marked this conversation as resolved.
Show resolved Hide resolved

return await targetCompletionSource.Task.WithTimeout(timeout).ConfigureAwait(false);
}
finally
Expand Down
74 changes: 63 additions & 11 deletions lib/PuppeteerSharp/ChromeTargetManager.cs
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Newtonsoft.Json.Linq;
using PuppeteerSharp.Helpers;
using PuppeteerSharp.Helpers.Json;
using PuppeteerSharp.Messaging;

Expand All @@ -25,17 +26,23 @@ internal class ChromeTargetManager : ITargetManager
private readonly List<string> _targetsIdsForInit = new();
private readonly TaskCompletionSource<bool> _initializeCompletionSource = new();

// Needed for .NET only to prevent race conditions between StoreExistingTargetsForInit and OnAttachedToTarget
private readonly int _targetDiscoveryTimeout;
private readonly TaskCompletionSource<bool> _targetDiscoveryCompletionSource = new();

public ChromeTargetManager(
Connection connection,
Func<TargetInfo, CDPSession, Target> targetFactoryFunc,
Func<TargetInfo, bool> targetFilterFunc)
Func<TargetInfo, bool> targetFilterFunc,
int targetDiscoveryTimeout = 0)
{
_connection = connection;
_targetFilterFunc = targetFilterFunc;
_targetFactoryFunc = targetFactoryFunc;
_logger = _connection.LoggerFactory.CreateLogger<ChromeTargetManager>();
_connection.MessageReceived += OnMessageReceived;
_connection.SessionDetached += Connection_SessionDetached;
_targetDiscoveryTimeout = targetDiscoveryTimeout;

_ = _connection.SendAsync("Target.setDiscoverTargets", new TargetSetDiscoverTargetsRequest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit of moving the call to Target.setDiscoverTargets in InitializeAsync instead of having it in the constructor as it is in puppeteer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't fully understand this, so I'll have to run more tests to get a better grip at it. But the test PuppeteerConnectTests.ShouldSupportTargetFilter was failing randomly. This test connects to a browser that already has 3 tabs open.

What I found was that the messages of the targets were being processed concurrently, even before the event handlers in AttachAsync were added. Again, ultimately this is due to the multi-threaded .NET vs single-threaded JS difference, as in JS the constructor and the event handlers would be added without any concurrent code.

I will get back to you with a more thorough explanation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the addition of _targetDiscoveryCompletionSource to ensure it has completed before anything else tries to consume _targetsIdsForInit which StoreExistingTargetsForInit populates.

Copy link
Contributor

@jnyrup jnyrup Jun 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea, if we move Target.setDiscoverTargets to InitializeAsync anyways, can we use modern async/await syntax and avoid _targetDiscoveryCompletionSource or am I missing some concurrency again?

public async Task InitializeAsync()
{
    var setDiscoverTargetsTask = _connection.SendAsync("Target.setDiscoverTargets", new TargetSetDiscoverTargetsRequest
    {
        Discover = true,
        Filter = new[]
        {
            new TargetSetDiscoverTargetsRequest.DiscoverFilter()
            {
                Type = "tab",
                Exclude = true,
            },
            new TargetSetDiscoverTargetsRequest.DiscoverFilter(),
        },
    });

    await _connection.SendAsync("Target.setAutoAttach", new TargetSetAutoAttachRequest()
    {
        WaitForDebuggerOnStart = true,
        Flatten = true,
        AutoAttach = true,
    }).ConfigureAwait(false);

    try
    {
        await setDiscoverTargetsTask.ConfigureAwait(false);
        StoreExistingTargetsForInit();
    }
    catch (Exception ex)
    {
        _logger.LogError(ex, "Target.setDiscoverTargets failed");
    }

    FinishInitializationIfReady();

    await _initializeCompletionSource.Task.ConfigureAwait(false);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnyrup I have reverted these changes and couldn't reproduce any failure in 100 executions of PuppeteerConnectTests.ShouldSupportTargetFilter. Let's see how it behaves in AppVeyor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out awaiting for _targetDiscoveryCompletionSource is needed before calling FinishInitializationIfReady(), otherwise FinishInitializationIfReady could execute before any targets have been added to the _targetsIdsForInit collection and the initialization would be considered completed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with how it's done now 👍

... but if we moved _connection.SendAsync("Target.setDiscoverTargets" to InitializeAsync again and awaited it, like in my example above, I'm failing to see how we can reach FinishInitializationIfReady before setDiscoverTargetsTask has completed, which should remove the need for _targetDiscoveryCompletionSource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnyrup had a closer look at your code above.

The change seems fine, and it simplifies a few things - for example it uses a try/catch which is more natural than checking t.IsFaulted. It's also closer to how FirefoxTargetManager works.

However, the _targetDiscoveryCompletionSource is still needed, to signal other threads that may be processing a message and executing OnAttachedToTarget.

It also has the downside of diverging from upstream.

I can commit these changes if you agree it's a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, it's those events messing with my brain again...
I now (again) see why _targetDiscoveryCompletionSource still is necessary.

I'll let kblok decide whether to call _connection.SendAsync("Target.setDiscoverTargets" in the constructor of InitializeAsync.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in this PR, _targetDiscoveryCompletionSource is more important than the location of Target.setDiscoverTargets. I would leave Target.setDiscoverTargets in the constructor as we have in upstream.

We are making some important changes (good changes). But it would be nice to see if we do need to move it to InitializeAsync in isolation. If we see that we need it, we can move it in a new PR.

This PR is looking great. I would merge it after the new WithTimeouts. Thank you @leonardo-fernandes, @jnyrup for the hard work here.

{
Expand All @@ -52,13 +59,20 @@ internal class ChromeTargetManager : ITargetManager
}).ContinueWith(
t =>
{
if (t.IsFaulted)
try
{
_logger.LogError(t.Exception, "Target.setDiscoverTargets failed");
if (t.IsFaulted)
{
_logger.LogError(t.Exception, "Target.setDiscoverTargets failed");
}
else
{
StoreExistingTargetsForInit();
}
}
else
finally
{
StoreExistingTargetsForInit();
_targetDiscoveryCompletionSource.SetResult(true);
}
},
TaskScheduler.Default);
Expand All @@ -83,7 +97,9 @@ await _connection.SendAsync("Target.setAutoAttach", new TargetSetAutoAttachReque
AutoAttach = true,
}).ConfigureAwait(false);

await _targetDiscoveryCompletionSource.Task.ConfigureAwait(false);
FinishInitializationIfReady();

await _initializeCompletionSource.Task.ConfigureAwait(false);
}

Expand Down Expand Up @@ -125,18 +141,32 @@ private void StoreExistingTargetsForInit()
}
}

private async void OnMessageReceived(object sender, MessageEventArgs e)
private async Task EnsureTargetsIdsForInit()
{
if (_targetDiscoveryTimeout > 0)
{
await _targetDiscoveryCompletionSource.Task.WithTimeout(_targetDiscoveryTimeout).ConfigureAwait(false);
}
else
{
await _targetDiscoveryCompletionSource.Task.ConfigureAwait(false);
}
}

private void OnMessageReceived(object sender, MessageEventArgs e)
{
try
{
switch (e.MessageID)
{
case "Target.attachedToTarget":
await OnAttachedToTarget(sender, e.MessageData.ToObject<TargetAttachedToTargetResponse>(true)).ConfigureAwait(false);
_ = OnAttachedToTargetHandlingExceptionsAsync(sender, e.MessageID, e.MessageData.ToObject<TargetAttachedToTargetResponse>(true));
return;

case "Target.detachedFromTarget":
OnDetachedFromTarget(sender, e.MessageData.ToObject<TargetDetachedFromTargetResponse>(true));
return;

case "Target.targetCreated":
OnTargetCreated(e.MessageData.ToObject<TargetCreatedResponse>(true));
return;
Expand All @@ -152,9 +182,7 @@ private async void OnMessageReceived(object sender, MessageEventArgs e)
}
catch (Exception ex)
{
var message = $"Browser failed to process {e.MessageID}. {ex.Message}. {ex.StackTrace}";
_logger.LogError(ex, message);
_connection.Close(message);
HandleExceptionOnMessageReceived(e.MessageID, ex);
}
}

Expand Down Expand Up @@ -182,9 +210,10 @@ private void OnTargetCreated(TargetCreatedResponse e)
}
}

private void OnTargetDestroyed(TargetDestroyedResponse e)
private async void OnTargetDestroyed(TargetDestroyedResponse e)
{
_discoveredTargetsByTargetId.TryRemove(e.TargetId, out var targetInfo);
await EnsureTargetsIdsForInit().ConfigureAwait(false);
FinishInitializationIfReady(e.TargetId);

if (targetInfo?.Type == TargetType.ServiceWorker && _attachedTargetsByTargetId.TryRemove(e.TargetId, out var target))
Expand Down Expand Up @@ -245,6 +274,7 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
if (targetInfo.Type == TargetType.ServiceWorker &&
_connection.IsAutoAttached(targetInfo.TargetId))
{
await EnsureTargetsIdsForInit().ConfigureAwait(false);
FinishInitializationIfReady(targetInfo.TargetId);
await silentDetach().ConfigureAwait(false);
if (_attachedTargetsByTargetId.ContainsKey(targetInfo.TargetId))
Expand All @@ -261,8 +291,10 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
if (_targetFilterFunc?.Invoke(targetInfo) == false)
{
_ignoredTargets.Add(targetInfo.TargetId);
await EnsureTargetsIdsForInit().ConfigureAwait(false);
FinishInitializationIfReady(targetInfo.TargetId);
await silentDetach().ConfigureAwait(false);
return;
leonardo-fernandes marked this conversation as resolved.
Show resolved Hide resolved
}

var existingTarget = _attachedTargetsByTargetId.TryGetValue(targetInfo.TargetId, out var target);
Expand Down Expand Up @@ -297,6 +329,7 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
}
}

await EnsureTargetsIdsForInit().ConfigureAwait(false);
_targetsIdsForInit.Remove(target.TargetId);

if (!existingTarget)
Expand All @@ -323,6 +356,25 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
}
}

private async Task OnAttachedToTargetHandlingExceptionsAsync(object sender, string messageId, TargetAttachedToTargetResponse e)
{
try
{
await OnAttachedToTarget(sender, e).ConfigureAwait(false);
}
catch (Exception ex)
{
HandleExceptionOnMessageReceived(messageId, ex);
}
}

private void HandleExceptionOnMessageReceived(string messageId, Exception ex)
{
var message = $"Browser failed to process {messageId}. {ex.Message}. {ex.StackTrace}";
_logger.LogError(ex, message);
_connection.Close(message);
}

private void FinishInitializationIfReady(string targetId = null)
{
if (targetId != null)
Expand Down
33 changes: 29 additions & 4 deletions lib/PuppeteerSharp/Helpers/TaskQueue.cs
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ namespace PuppeteerSharp.Helpers
internal sealed class TaskQueue : IDisposable, IAsyncDisposable
{
private readonly SemaphoreSlim _semaphore;
private readonly AsyncLocal<bool> _held = new AsyncLocal<bool>();
private int _disposed;

internal TaskQueue() => _semaphore = new SemaphoreSlim(1);
Expand All @@ -18,7 +19,11 @@ public void Dispose()
return;
}

_semaphore.Wait();
if (!_held.Value)
{
_semaphore.Wait();
}

_semaphore.Dispose();
}

Expand All @@ -29,7 +34,10 @@ public async ValueTask DisposeAsync()
return;
}

await _semaphore.WaitAsync().ConfigureAwait(false);
if (!_held.Value)
{
await _semaphore.WaitAsync().ConfigureAwait(false);
}

_semaphore.Dispose();
}
Expand All @@ -39,11 +47,13 @@ internal async Task<T> Enqueue<T>(Func<Task<T>> taskGenerator)
await _semaphore.WaitAsync().ConfigureAwait(false);
try
{
_held.Value = true;
return await taskGenerator().ConfigureAwait(false);
}
finally
{
_semaphore.Release();
TryRelease(_semaphore);
_held.Value = false;
}
}

Expand All @@ -52,11 +62,26 @@ internal async Task Enqueue(Func<Task> taskGenerator)
await _semaphore.WaitAsync().ConfigureAwait(false);
try
{
_held.Value = true;
await taskGenerator().ConfigureAwait(false);
}
finally
{
_semaphore.Release();
TryRelease(_semaphore);
_held.Value = false;
}
}

private void TryRelease(SemaphoreSlim semaphore)
{
try
{
semaphore.Release();
}
catch (ObjectDisposedException)
{
// If semaphore has already been disposed, then Release() will fail
// but we can safely ignore it
}
}
}
Expand Down