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

handle memory leaks #6

Closed
wants to merge 2 commits into from
Closed

Conversation

dsadinoff
Copy link

In regards to https://rt.cpan.org/Public/Bug/Display.html?id=93693
It's probably not that surprising that there's some real leaks in this module, expecially $my->{$self} = ...

attached is an implementation of a reset() method to clear the memory.

@mgreter
Copy link
Contributor

mgreter commented Mar 10, 2014

I can confirm the leaks. I tried to add an automatic way instead of needing to call reset myself (since it would reset all instances). HTML::TreeBuilder uses Scalar::Util::weaken (if available) to solve circular dependencies with parent nodes. It actually turned out to be much easier. Just had to add a deconstructor to free the global resources stored in $my: mgreter@4f8fd2a

@dsadinoff
Copy link
Author

Yes, that's a much better approach.

At least my leak test seems to hold up.

Cheers!

@mgreter
Copy link
Contributor

mgreter commented Jul 5, 2014

I guess this can be closed as these commits were already included with #5.

@ingydotnet
Copy link
Owner

I think these changes got included in the application of #5.

Everything should be on CPAN pQuery-0.12

Thanks!

@ingydotnet ingydotnet closed this Jul 5, 2014
@mgreter
Copy link
Contributor

mgreter commented Jul 5, 2014

@ingydotnet: I guess there was some confusion about this issue. I see you've added the documentation for the reset method back (310cc92). This is no longer needed as I was able to solve this by implementing DESTROY for the object, so the resources should be deallocated automatically! See my other commit from #5 (mgreter@3c57a37) which removed the reset method again! Basically what's left from @dsadinoff are the test cases. I guess other than that there was no real use for the reset method!?

It might be worth mentioning that this will only work (afaik) if weaken from Scalar::Util is available. pQuery could enforce that for HTML::TreeBuilder.

@ingydotnet
Copy link
Owner

@mgreter, can you possibly update the doc for me? It's in doc/pQuery.swim. Other docs get generated by it, but I'll do that part. The swim format should be fairly self-explanatory.

I didn't really go over the code, since I'm working on a ton of other stuff, but I want to get the contributions released.

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.

None yet

3 participants