Bug #56838 for XML-Twig: reference loop through twig_ignore

Thu Apr 22 20:36:00 2010 andrew [...] pimlott.net - Ticket created

CC:	Andrew Pimlott <andrew [...] pimlott.net>
Subject:	reference loop through twig_ignore_action
Date:	Thu, 22 Apr 2010 17:35:29 -0700
To:	bug-XML-Twig [...] rt.cpan.org
From:	Andrew Pimlott <andrew [...] pimlott.net>

If you run the following code under XML::Twig 3.35, my $twig = XML::Twig->new( ignore_elts => { x => 1 } ); $twig->parse("<x/>"); $twig->dispose(); print "$twig $twig->{twig_ignore_action}\n"; you will find that $twig->{twig_ignore_action} points back to $twig. Thus, $twig will never get garbage collected. The line my $action= shift || 1; in the ignore function at line 3803 looks highly suspect. Andrew

Thu Apr 22 21:12:22 2010 andrew [...] pimlott.net - Correspondence added

Subject:	Re: [rt.cpan.org #56838] AutoReply: reference loop through twig_ignore_action
Date:	Thu, 22 Apr 2010 18:12:11 -0700
To:	bug-XML-Twig <bug-xml-twig [...] rt.cpan.org>
From:	Andrew Pimlott <andrew [...] pimlott.net>

Follow-up suggestion: The reason this bug was so painful to me was not the memory leak; it was that because the twig stayed alive, my handlers stayed alive, and the handlers had references to objects that needed to destruct after parsing finished. I propose that ->dispose unset the twig's handlers. That way, even if the twig itself doesn't go away (which would of course be a bug, but bugs happen), at least my handlers would be garbage collected. Andrew

Fri Apr 23 13:53:25 2010 xmltwig [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #56838] reference loop through twig_ignore_action
Date:	Fri, 23 Apr 2010 11:53:13 -0600
To:	bug-XML-Twig [...] rt.cpan.org
From:	Michel Rodriguez <xmltwig [...] gmail.com>

I think you're right, the line $action= shift || 1 should be $action= $_[2] || 1, @_ has not been changed at that point, so its first element is the twig itself. I have to write the tests. As for unsetting the handlers in dispose, yes it can be done. I'll do this probably early next week. Thanks for the bug report. -- mirod

Fri Apr 23 13:53:26 2010 The RT System itself - Status changed from 'new' to 'open'

Fri Apr 23 16:40:15 2010 andrew [...] pimlott.net - Correspondence added

Subject:	Re: [rt.cpan.org #56838] reference loop through twig_ignore_action
Date:	Fri, 23 Apr 2010 13:39:58 -0700
To:	bug-XML-Twig <bug-xml-twig [...] rt.cpan.org>
From:	Andrew Pimlott <andrew [...] pimlott.net>

Excerpts from xmltwig@gmail.com via RT's message of Fri Apr 23 10:53:26 -0700 2010: Show quoted text

> I think you're right, the line $action= shift || 1 should be $action= > $_[2] || 1, @_ has not been changed at that point, so its first > element is the twig itself. I have to write the tests. As for > unsetting the handlers in dispose, yes it can be done.

Awesome, it's really nice to see you are still committed to this code! For now, I'm using the obvious work-around: $twig->{twig_ignore_action} = undef; Andrew

Tue May 04 17:09:14 2010 andrew [...] pimlott.net - Correspondence added

Subject:	Re: [rt.cpan.org #56838] reference loop through twig_ignore_action
Date:	Tue, 04 May 2010 14:09:00 -0700
To:	bug-XML-Twig <bug-xml-twig [...] rt.cpan.org>
From:	Andrew Pimlott <andrew [...] pimlott.net>

I just happened to look at the Twig DESTROY function. It looks like there is an attempt to completely clear out the Twig object: $t={}; # prevents memory leaks (especially when using mod_perl) undef $t; It it had worked, my memory leak would not have happened. However, this code doesn't actually do anything useful. It points $t to some new hash, then unsets it. This has no effect on the Twig object. For that, you probably want to empty the hash representing the Twig: %$t = (); If you do that (and it seems like a good idea to me), you can remove all the undef lines as they will be redundant. Andrew Excerpts from xmltwig@gmail.com via RT's message of Fri Apr 23 10:53:26 -0700 2010: Show quoted text

> <URL: http://rt.cpan.org/Ticket/Display.html?id=56838 > > > I think you're right, the line $action= shift || 1 should be $action= > $_[2] || 1, @_ has not been changed at that point, so its first > element is the twig itself. I have to write the tests. As for > unsetting the handlers in dispose, yes it can be done. > > I'll do this probably early next week. > > Thanks for the bug report. >

Tue May 18 05:57:41 2010 MIROD [...] cpan.org - Correspondence added

On Tue May 04 17:09:14 2010, andrew@pimlott.net wrote: Show quoted text

> I just happened to look at the Twig DESTROY function. It looks like > there is an attempt to completely clear out the Twig object: > > $t={}; # prevents memory leaks (especially when using mod_perl) > undef $t; > > It it had worked, my memory leak would not have happened. However, > this > code doesn't actually do anything useful. It points $t to some new > hash, then unsets it. This has no effect on the Twig object. For > that, > you probably want to empty the hash representing the Twig: > > %$t = (); > > If you do that (and it seems like a good idea to me), you can remove > all > the undef lines as they will be redundant.

I did %$t = (); although I am not quite certain that really helps. That's in XML::Twig 3.35 I would need more time to investigate potential memory leaks due to handlers not being freed, but I don't have much at the moment. Let me know if the new version fixes the problem for you, and if not I'll have a look at it. Thanks __ mirod

Mon Jun 07 14:56:34 2010 andrew [...] pimlott.net - Correspondence added

Subject:	Re: [rt.cpan.org #56838] reference loop through twig_ignore_action
Date:	Mon, 07 Jun 2010 11:56:24 -0700
To:	bug-XML-Twig <bug-xml-twig [...] rt.cpan.org>
From:	Andrew Pimlott <andrew [...] pimlott.net>

Sorry not to follow up sooner. I'm working with XML::Twig again, and I tested your changes in the latest 3.35, and they do fix things for me. Thanks! Andrew Excerpts from MIROD via RT's message of Tue May 18 02:57:41 -0700 2010: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=56838 > > > On Tue May 04 17:09:14 2010, andrew@pimlott.net wrote:

> > I just happened to look at the Twig DESTROY function. It looks like > > there is an attempt to completely clear out the Twig object: > > > > $t={}; # prevents memory leaks (especially when using mod_perl) > > undef $t; > > > > It it had worked, my memory leak would not have happened. However, > > this > > code doesn't actually do anything useful. It points $t to some new > > hash, then unsets it. This has no effect on the Twig object. For > > that, > > you probably want to empty the hash representing the Twig: > > > > %$t = (); > > > > If you do that (and it seems like a good idea to me), you can remove > > all > > the undef lines as they will be redundant.

> > I did %$t = (); although I am not quite certain that really helps. > That's in XML::Twig 3.35 > > I would need more time to investigate potential memory leaks due to > handlers not being freed, but I don't have much at the moment. Let me > know if the new version fixes the problem for you, and if not I'll have > a look at it. > > Thanks > __ > mirod

Mon Feb 28 13:03:36 2011 MIROD [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Feb 28 13:03:36 2011 MIROD [...] cpan.org - Fixed in 3.35 added

Bug #56838 for XML-Twig: reference loop through twig_ignore_action