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

chore: fix impl to api name memory leak #1604

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

mxschmitt
Copy link
Member

fixes #1602

@nay-kang
Copy link

nay-kang commented Nov 2, 2022

I had face this memory leak for a week. thank you for fixing it.
but I'm confusing with this code:

  • there is seems no call to from_maybe_impl function with visited argument
  • it has no benift to create a visited inside this function. in the previous version the shared visisted argument could cache obj between calls to speed up but will cause memory leak.

so I guess if we can remove the visited argument. or using an instance attribute belongs to BrowserContext then we can close BrowserContext to relase memory.

and there has another code could cause memory leak playwright/_impl/_js_handle.py

@dataclass
class VisitorInfo:
    visited: Map[Any, int] = Map()
    last_id: int = 0

    def visit(self, obj: Any) -> int:
        assert obj not in self.visited
        self.last_id += 1
        self.visited[obj] = self.last_id
        return self.last_id

the visited attribute in the VisitorInfo is a class attribute, which will share between instance and will not release memory until reload the module.

all this code are belongs to the same commit:ed13a53281bc213e1a4341c50d47fbc65670288f

@mxschmitt
Copy link
Member Author

I see one, see here.

Will create a follow-up with this potential VisitorInfo leak, thanks for pointing this out.

@nay-kang
Copy link

nay-kang commented Nov 3, 2022

hi @mxschmitt ,you memtioned the from_maybe_impl function with visited argument call here

o[name] = self.from_maybe_impl(value, visited)

is a recursive call to himself, so I think it could run well without visisted argument

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.

[BUG] CDPSession.on() causes memory leak
3 participants