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 1 commit
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
44 changes: 23 additions & 21 deletions lib/PuppeteerSharp/ChromeTargetManager.cs
View file
Open in desktop
Expand Up @@ -38,8 +38,8 @@ internal class ChromeTargetManager : ITargetManager
_targetFactoryFunc = targetFactoryFunc;
_logger = _connection.LoggerFactory.CreateLogger<ChromeTargetManager>();
_connection.MessageReceived += OnMessageReceived;
_connection.SessionDetached += Connection_SessionDetached;
_connection.SessionDetached += Connection_SessionDetached;

_ = _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.

{
Discover = true,
Expand Down Expand Up @@ -137,29 +137,12 @@ private void StoreExistingTargetsForInit()

private void OnMessageReceived(object sender, MessageEventArgs e)
{
Action<Exception> handleException = (Exception ex) =>
{
var message = $"Browser failed to process {e.MessageID}. {ex.Message}. {ex.StackTrace}";
_logger.LogError(ex, message);
_connection.Close(message);
};

try
{
switch (e.MessageID)
{
case "Target.attachedToTarget":
Task.Run(async () =>
{
try
{
await OnAttachedToTarget(sender, e.MessageData.ToObject<TargetAttachedToTargetResponse>(true)).ConfigureAwait(false);
}
catch (Exception ex)
{
handleException(ex);
}
});
_ = OnAttachedToTargetHandlingExceptions(sender, e.MessageID, e.MessageData.ToObject<TargetAttachedToTargetResponse>(true)).ConfigureAwait(false);
leonardo-fernandes marked this conversation as resolved.
Show resolved Hide resolved
return;

case "Target.detachedFromTarget":
Expand All @@ -181,7 +164,7 @@ private void OnMessageReceived(object sender, MessageEventArgs e)
}
catch (Exception ex)
{
handleException(ex);
HandleExceptionOnMessageReceived(e.MessageID, ex);
}
}

Expand Down Expand Up @@ -354,6 +337,25 @@ private async Task OnAttachedToTarget(object sender, TargetAttachedToTargetRespo
}
}

private async Task OnAttachedToTargetHandlingExceptions(object sender, string messageId, TargetAttachedToTargetResponse e)
leonardo-fernandes marked this conversation as resolved.
Show resolved Hide resolved
{
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
28 changes: 16 additions & 12 deletions lib/PuppeteerSharp/Page.cs
View file
Open in desktop
Expand Up @@ -671,25 +671,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