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.

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

Fix deadlock with executionContextMu #636

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Nov 7, 2022

executionContextMu isn't needed to be locked and unlocked in this (WaitForFunction) method as it doesn't r/w to executionContext or documentHandle. This function later calls an unexported method with the same name, which does require access to executionContext.

A deadlock was occurring later in waitForExecutionContext since this method was holding on to the lock.

Closes: #635

executionContextMu isn't needed to be locked and unlocked in this
(WaitForFunction) method as it doesn't r/w to executionContext or
documentHandle. This function later calls an unexported method with the
same name, which does require access to executionContext.

A deadlock was occurring later in waitForExecutionContext since
this method was holding on to the lock.

Closes: #635
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice! However, there is still a devil lurking in those waters. The following deadlocks after a while:

go test ./tests -run='TestPageWaitForFunction' -count=100 -timeout=10s

We can look at this in-depth later.

@ankur22 ankur22 merged commit 7a45ec7 into main Nov 7, 2022
@ankur22 ankur22 deleted the fix/635-execCtxMu-deadlock branch November 7, 2022 13:37
ankur22 added a commit that referenced this pull request Nov 11, 2022
executionContextMu isn't needed to be locked and unlocked in this
(WaitForFunction) method as it doesn't r/w to executionContext or
documentHandle. This function later calls an unexported method with the
same name, which does require access to executionContext.

A deadlock was occurring later in waitForExecutionContext since
this method was holding on to the lock.

Closes: #635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock in TestPageWaitForFunction tests
2 participants