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

[BUG] Fail to close context when running inside a larger loop #1886

Closed
AlmogBaku opened this issue Apr 28, 2023 · 2 comments
Closed

[BUG] Fail to close context when running inside a larger loop #1886

AlmogBaku opened this issue Apr 28, 2023 · 2 comments
Labels

Comments

@AlmogBaku
Copy link

AlmogBaku commented Apr 28, 2023

Context:

  • Playwright Version: 1.32.1
  • Operating System: macos
  • Python version: [e.g. 3.7, 3.9]
  • Browser: all

Describe the bug

I'm running a gRPC server using a loop, and then using playwright to fetch some URL: https://github.com/hwchase17/langchain/blob/e3b7a20454cea592fc6d0a0d91c36206e8ad6790/langchain/document_loaders/url_playwright.py#L62

when the context is exiting, this error is thrown
RuntimeError: Set changed size during iteration

this is happening due to an old upstream bug in cpython python/cpython#80788

we need to refactor the context manager to close itself differently

for t in [t for t in tasks if not (t.done() or t.cancelled())]:

TBH, I don't know why we even do that :O

@mxschmitt
Copy link
Member

mxschmitt commented May 2, 2023

Playwright is only supported in a single thread and does not allow any thread-sharing operations. Is this maybe the reason why this comes up?

Would it be possible to provide a minimal reproduction which demonstrates this issue?

Most likely unrelated: I see that inside your loop you call browser.new_page() but never close the page. You need to call page.close() at the end of your loop to not have any memory leak during your for loop. (Yes browser.close() will also clean up all the pages, and contexts, but if your for loop contains 1000 items, its likely to create a lot of unnecessary memory.)

It was added as part of #1430. Would have to dig deeper to understand the reason behind this change.

@mxschmitt
Copy link
Member

Closing as part of the triage process since it seemed stale. Please create a new issue with a detailed reproducible or feature request if you still face issues.

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

No branches or pull requests

2 participants