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

Browser Pool does not await _createPageForBrowser #2789

Open
1 task
danielcrabtree opened this issue Jan 4, 2025 · 5 comments · May be fixed by #2860
Open
1 task

Browser Pool does not await _createPageForBrowser #2789

danielcrabtree opened this issue Jan 4, 2025 · 5 comments · May be fixed by #2860
Assignees
Labels
bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team.

Comments

@danielcrabtree
Copy link

Which package is this bug report for? If unsure which one to select, leave blank

@crawlee/browser-pool

Issue description

The newPage and newPageInNewBrowser functions in BrowserPool both return _createPageForBrowser() without awaiting the result. This causes the browser to hang under certain difficult to reproduce race conditions - adding await to these calls appears to resolve these race conditions.

Code sample

No response

Package version

3.12.1

Node.js version

22

Operating system

No response

Apify platform

  • Tick me if you encountered this issue on the Apify platform

I have tested this on the next release

No response

Other context

No response

@danielcrabtree danielcrabtree added the bug Something isn't working. label Jan 4, 2025
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Jan 4, 2025
@B4nan B4nan self-assigned this Jan 6, 2025
@theanuragg
Copy link

hey @B4nan , this issue is assigned to you!.. are you working on this if not then I would love to work on this

@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

Feel free to look into it.

@theanuragg
Copy link

my approach would be By adding await before _createPageForBrowser() in both functions

@theanuragg theanuragg linked a pull request Feb 25, 2025 that will close this issue
@B4nan
Copy link
Member

B4nan commented Feb 25, 2025

I am having a hard time understanding how the added awaits in #2860 could help. Both places are async, we are not ignoring any promise in there. By adding the await we only make sure the promise is finished before we return.

@danielcrabtree can you confirm #2860 is what helped on your end? I don't mind merging that, it just feels weird that this could cause a race condition.

@danielcrabtree
Copy link
Author

I was specifically having the issue with newPage. I'm unsure if the problem occurs with newPageInNewBrowser as I wasn't using that and since it does not appear to use limiter, maybe race conditions are not an issue in that case?

My understanding of the issue is that limiter is designed to run the inline async function with concurrency of 1 to avoid race conditions between parallel browser actions.

By not awaiting _createPageForBrowser within the limiter context, this opens up the possibility of race conditions between that operation and parallel browser launches as the _createPageForBrowser method can (and in my case sometimes was) complete after exiting from the limiter context.

Adding the await to the call to _createPageForBrowser in newPage fixed the race condition for me. I was never able to produce a minimum example that reliably triggered the issue to prove this fixed it, but it's been over a month now and I have yet to see the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants