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: await on browser pool _createpageforbrowser #2860

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: await on browser pool _createpageforbrowser #2860

wants to merge 3 commits into from

Conversation

theanuragg
Copy link

This PR fixes an issue where BrowserPool could hang due to _createPageForBrowser() returning a Promise
without being awaited. By explicitly awaiting this function in both newPage and newPageInNewBrowser,
we prevent race conditions that could cause browsers to remain stuck.

Changes:

  • Added await to _createPageForBrowser()
  • Wrapped in try-catch for better error handling

Fixes #2789

@janbuchar janbuchar requested a review from B4nan February 25, 2025 11:56
@theanuragg theanuragg changed the title await on browser pool _createpageforbrowser bug: await on browser pool _createpageforbrowser Feb 25, 2025
@theanuragg theanuragg changed the title bug: await on browser pool _createpageforbrowser fix: await on browser pool _createpageforbrowser Feb 25, 2025
B4nan
B4nan reviewed Feb 25, 2025
View reviewed changes
Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

are you sure the changes you did are fixing anything? i dont think they will have any effect. did you face this issue yourself? or how are you testing the changes have any effect?

packages/browser-pool/src/browser-pool.ts Outdated
@@ -471,7 +471,7 @@ export class BrowserPool<

const browserController = await this._launchBrowser(id, { launchOptions, browserPlugin });
tryCancel();
return this._createPageForBrowser(id, browserController, pageOptions);
return await this._createPageForBrowser(id, browserController, pageOptions);
Copy link
Member

Choose a reason for hiding this comment

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

i don't see how this could have any effect, the method is async, this wasn't an ignored promise (and if it was, the issue was on a higher level)

packages/browser-pool/src/browser-pool.ts Outdated
@@ -451,7 +451,7 @@ export class BrowserPool<
browserController = await this._launchBrowser(id, { browserPlugin, proxyTier, proxyUrl });
tryCancel();

return this._createPageForBrowser(id, browserController, pageOptions, proxyUrl);
return await this._createPageForBrowser(id, browserController, pageOptions, proxyUrl);
Copy link
Member

Choose a reason for hiding this comment

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

and i am also not convinced this can help, the inner function was also async, returning from inside an async function is enough.

@theanuragg
Copy link
Author

theanuragg commented Feb 25, 2025 •
edited
Loading

hey @B4nan, since you mention the browser hangs due to difficult-to-reproduce the potential cause can be multiple asynchronous calls interfering

and instead of await I need to make sure synchronization and properly handle concurrency issues.

@B4nan B4nan mentioned this pull request Feb 25, 2025
Open
1 task
@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

the indents are messed up, please fix that so the git diff is readable

@theanuragg
Copy link
Author

Hey @B4nan, can you send a screenshot of the messed-up indents in the git diff? I'd like to see what it looks like on your end.

@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

now i see you reformatted a lot of unrelated things. please revert all the formatting changes.

@theanuragg
Copy link
Author

I made three commits: first for await, second for fixing concurrent calls, and third for fixing the indentation issue. Which one do you want me to revert?

@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

All formatting changes. You reformatted the whole file, none of those changes are relevant to what you are trying to fix, so please revert them. The PR diff needs to stay clear so I can see what you actually changed. Note that we use biome in this repository, its enough to run yarn format, this looks like you use something else, ignoring the rules we have in place for formatting.

@theanuragg
Copy link
Author

I’ll revert all the formatting changes and make sure to follow the rules using Biome. heads-up for pointing out

@danielcrabtree
Copy link

danielcrabtree commented Feb 26, 2025 •
edited
Loading

I'm still seeing all the formatting changes in the diff, so it's hard to determine if there have been any other changes besides the fix I applied locally. But the await line commented on by @B4nan is the change that I made that resolved the race condition with newPage. See my longer discussion on #2789 for further details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@B4nan B4nan B4nan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Browser Pool does not await _createPageForBrowser
3 participants
@theanuragg @B4nan @danielcrabtree