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 14 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
20 changes: 6 additions & 14 deletions lib/PuppeteerSharp/Browser.cs
View file
Open in desktop
Expand Up @@ -201,12 +201,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 +213,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
56 changes: 46 additions & 10 deletions lib/PuppeteerSharp/ChromeTargetManager.cs
View file
Open in desktop
Expand Up @@ -25,6 +25,9 @@ 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 TaskCompletionSource<bool> _targetDiscoveryCompletionSource = new();

public ChromeTargetManager(
Connection connection,
Func<TargetInfo, CDPSession, Target> targetFactoryFunc,
Expand Down Expand Up @@ -52,13 +55,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 +93,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 +137,20 @@ private void StoreExistingTargetsForInit()
}
}

private async void OnMessageReceived(object sender, MessageEventArgs e)
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 +166,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 +194,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 _targetDiscoveryCompletionSource.Task.ConfigureAwait(false);
Copy link
Member

@kblok kblok Jun 12, 2023 •
edited

Choose a reason for hiding this comment

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

Although we are trying to enforce this. What do you think about using WithTimeout, and throwing an exception? If we fail to complete this task somehow, the code will get stuck here.

The same for the other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that _targetDiscoveryCompletionSource will be set if and only if the message Target.setDiscoverTargets completes. I can't think of any scenario where it wouldn't complete.

There's no code that uses a timeout value in ChromeTargetManager. Where should I derive the timeout value from?

Copy link
Member

Choose a reason for hiding this comment

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

Let's use Puppeteer.DefaultTimeout. It's quite a long timeout. We want to prevent the code from hanging.

FinishInitializationIfReady(e.TargetId);

if (targetInfo?.Type == TargetType.ServiceWorker && _attachedTargetsByTargetId.TryRemove(e.TargetId, out var target))
Expand Down Expand Up @@ -245,6 +258,7 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
if (targetInfo.Type == TargetType.ServiceWorker &&
_connection.IsAutoAttached(targetInfo.TargetId))
{
await _targetDiscoveryCompletionSource.Task.ConfigureAwait(false);
FinishInitializationIfReady(targetInfo.TargetId);
await silentDetach().ConfigureAwait(false);
if (_attachedTargetsByTargetId.ContainsKey(targetInfo.TargetId))
Expand All @@ -261,8 +275,10 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
if (_targetFilterFunc?.Invoke(targetInfo) == false)
{
_ignoredTargets.Add(targetInfo.TargetId);
await _targetDiscoveryCompletionSource.Task.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 +313,7 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
}
}

await _targetDiscoveryCompletionSource.Task.ConfigureAwait(false);
_targetsIdsForInit.Remove(target.TargetId);

if (!existingTarget)
Expand All @@ -323,6 +340,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
28 changes: 16 additions & 12 deletions lib/PuppeteerSharp/Page.cs
View file
Open in desktop
Expand Up @@ -667,25 +667,29 @@ public async Task<byte[]> ScreenshotDataAsync(ScreenshotOptions options)
public Task<string> GetTitleAsync() => MainFrame.GetTitleAsync();

/// <inheritdoc/>
public Task CloseAsync(PageCloseOptions options = null)
public async Task CloseAsync(PageCloseOptions options = null)
{
if (!(Client?.Connection?.IsClosed ?? true))
if (Client?.Connection?.IsClosed ?? true)
{
var runBeforeUnload = options?.RunBeforeUnload ?? false;
_logger.LogWarning("Protocol error: Connection closed. Most likely the page has been closed.");
return;
}

if (runBeforeUnload)
{
return Client.SendAsync("Page.close");
}
var runBeforeUnload = options?.RunBeforeUnload ?? false;

return Client.Connection.SendAsync("Target.closeTarget", new TargetCloseTargetRequest
if (runBeforeUnload)
{
await Client.SendAsync("Page.close").ConfigureAwait(false);
}
else
{
await Client.Connection.SendAsync("Target.closeTarget", new TargetCloseTargetRequest
{
TargetId = Target.TargetId,
}).ContinueWith(task => ((Target)Target).CloseTask, TaskScheduler.Default);
}
}).ConfigureAwait(false);

_logger.LogWarning("Protocol error: Connection closed. Most likely the page has been closed.");
return Task.CompletedTask;
await ((Target)Target).CloseTask.ConfigureAwait(false);
Comment on lines +686 to +691
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 noticing that we now only execute ((Target)Target).CloseTask if Client.Connection.SendAsync("Target.closeTarget" completes successfully.
Should we still execute ((Target)Target).CloseTask if Client.Connection.SendAsync("Target.closeTarget" throws an exception?

Copy link
Member

Choose a reason for hiding this comment

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

@jnyrup this matches upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, I based it on upstream.

If Target.closeTarget fails, it's very likely that TargetManager_TargetGone will not be invoked, and the await of CloseTask would block the code indefinitely.

}
}

/// <inheritdoc/>
Expand Down