Bug #42503 for Test-Weaken: suggest skipping "permanent" objects

Sun Jan 18 16:18:22 2009 user42 [...] zip.com.au - Ticket created

Subject:	suggest skipping "permanent" objects
Date:	Mon, 19 Jan 2009 08:16:35 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

In some of my code I end up with what are basically permanent objects, held in a cache or as constants or whatnot. It'd be nice if Test::Weaken::poof allowed for some parts, ie. sub-objects, of the things probed to not destruct when weakened. (Unless that works already and I just failed to understand the docs :-) I don't know how such a thing would look. Maybe nominated objects, maybe determined only after the poof constructing, in some kind of hook. Maybe all objects of a given class would be permanent sometimes. Maybe poof could automatically notice when an object class uses Class::Singleton and an object ends up as its singleton instance (allowing other instances to exist too, even if that's not the normal pattern for a singleton class).

Sun Jan 18 22:42:22 2009 jkegl [...] cpan.org - Correspondence added

In array context, poof returns the objects which did not get freed. (see the document for details.) It is, of course, very common for an object to contain references to "permanent" objects, references that should not go away even when memory for the main object is free. All these "permanent" objects should be in the lists returned by poof() in array context. Using poof() determine if your scheme for freeing memory is correct does become more complex in this context. You have to examine the objects returned by poof() to see if only the "right" ones were left unfreed. On the assumption that I understood the question and that this is responsive, I'm going to close this ticket. Feel free to reopen it if I missed the point. Also, if you want to discuss alternatives, email me at the cpan.org address. I'll put rereading the Test::Weaken document on my to do list, to see if I could make things clearer. Perhaps the document needs a "Tricks" section, pointing out some of these things.

Sun Jan 18 22:42:23 2009 The RT System itself - Status changed from 'new' to 'open'

Sun Jan 18 22:44:57 2009 jkegl [...] cpan.org - Correspondence added

Responded, pointing out how to handle "permanent" objects using Test::Weaken. In future, I may revise the document to answer this question directly and more clearly.

Sun Jan 18 22:44:58 2009 jkegl [...] cpan.org - Status changed from 'open' to 'resolved'

Fri Jan 23 19:07:54 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Sat, 24 Jan 2009 09:01:50 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > <URL: https://rt.cpan.org/Ticket/Display.html?id=42503 > > > Perhaps the document needs a "Tricks" section, pointing out some of > these things.

Sounds likely. I'd kinda thought of the array return as only for diagnostics rather than post-processing. One thing I struck was that my persistent/external/permanent etc objects contain sub-part arrayrefs or hashrefs, normally not meant to see the light of day of course. Those fragments get included in the unfreed return. Identifying them as parts of the "correctly unfreed" seems a little tedious. But maybe there's tricks for that the docs could mention.

Fri Jan 23 19:07:55 2009 The RT System itself - Status changed from 'resolved' to 'open'

Sat Jan 24 15:29:56 2009 jkegl [...] cpan.org - Correspondence added

How about a "filter" argument to poof()? Any reference passed to poof will not count as unfreed. The filtered reference would be followed recursively, in the same manner that poof() currently follows the reference to its test object -- hashes, array and weak and strong references would all be followed. All references found in this recursion would also be filtered references. Cycles would be handled gracefully. The count returned by poof() in scalar context will not include references which were unfreed, but filtered. Neither will the list of unfreed references returned in array context. poof() will not check that its test object actually refers to any of the filtered references. poof() will also not consider it an error if a filtered reference is freed. I believe this behavior follows the principle of "least surprise". Another benefit of this behavior is that, if multiple references need to be filtered, they can be passed as a reference to an array of references. The interface is now complex enough that I'll allow arguments to be passed to poof() as a hash ref. It will allow the following named arguments: "constructor", "destructor" and "filter". The value of the "constructor" and "destructor" named arguments will be coderefs, as before. The value of the "filter" named argument will be a reference which, along with all its referents followed recursively, is to be excluded from the unfreed counts and lists. The old interface will continue to be accepted. Could you supply test cases? As with the other bug, I want to use it for cpantesters, so the test cases shouldn't rely on non-core modules. Just the bare routines are needed, I'll add my own wrappering. If you don't supply test cases, I'll create my own, but (again) the advantage is that if you supply the test cases, we know that I've added the feature that you want, not just my idea of the feature you want. thanks, jeffrey

Sat Jan 24 15:31:30 2009 jkegl [...] cpan.org - Correspondence added

Subject:

New "Tricks" section in documentation

In 1.001_004, I did add a "Tricks" section to the documentation. -- jeffrey

Sat Jan 24 15:31:40 2009 jkegl [...] cpan.org - Taken

Wed Jan 28 16:34:56 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Thu, 29 Jan 2009 08:14:19 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > Could you supply test cases?

The couple of lines below is the sort of thing I struck. The filter option in this case would be a hypothetical based on my idea that all objects of a certain class persist, instead of nominating them individually. I expect both by-class and individual have their use. I could also imagine running together a couple of filter conditions. I guess some "||"s in a func can do that, though maybe the interface could offer an easier way. The dump of @weaken in the code below only seems to show the arrayref guts of MyGlobal things. I think I got that from my real code too. I wasn't quite sure why the actual MyGlobal objects didn't appear in the "unfreed strong" of $weaken[3]. They're still in the %cache (commented-out dump line).

Message body is not shown because sender requested not to inline it.

Thu Jan 29 14:19:05 2009 jkegl [...] cpan.org - Correspondence added

Subject:

Follow references in permanent objects?

Thanks for the test cases. I've downloaded the file and will get to work on this. There's an issue about whether I should follow references in a filtered objects while checking for memory leaks. That is, a constructor might take a permanent object and add objects to it. It's not 100% clear whether that counts as memory that should be released. It certainly *COULD* cause a memory leak. On the other hand, such added memory could be thought of as part of the "permanent" object, to be release when and if the permanent object is released. My inclination is to not follow by default, so that by default such leaks would not be detected. This seems to obey the principle of "least surprise". I would also add a "follow" argument which allows the user to follow references in permanent objects for checking purposes.

Tue Feb 03 00:40:13 2009 jkegl [...] cpan.org - Correspondence added

Subject:

About this feature request and 2.000000 upcoming

Before I add this feature, I'm going to complete the upcoming new major release 2.000000. 2.000000 will not have this feature, but when I rewrote the internals of Test::Weaken for it, I had this in mind. I factored out the code for finding all the memory objects contained in a test object. I also added the named argument format to the new interface. 2.000000 I hope will be on CPAN in a matter of days. I've just uploaded a developer's release, 1.003_000, which passes all regression tests, fixes bug #42903 and has the documentation for 2.000000 in late draft form.

Tue Feb 03 00:40:37 2009 jkegl [...] cpan.org - Status changed from 'open' to 'stalled'

Tue Feb 03 17:56:04 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] Follow references in permanent objects?
Date:	Wed, 04 Feb 2009 09:53:37 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > a constructor might take a permanent object and add objects to it.

That may be outside the scope of what can easily be tested. I think I've had situations both where I've hung data on another object, and where the other object keeps it's own records of things about me. The first is a leak if not cleaned up, the second is a matter for the other object. But which is which would be nearly indistinguishable.

Tue Feb 03 17:56:05 2009 The RT System itself - Status changed from 'stalled' to 'open'

Thu Feb 26 16:14:31 2009 jkegl [...] cpan.org - Correspondence added

I've just put a developer's release on CPAN that fixes this. It is 2.001_000. Comments are welcome if you get a chance. I'm keeping the bug open, probably until I do an non-developer's release with the fix. I named the option 'ignore' in keeping with the track/follow/ignore nomenclature I use in the 2.000000 documentation. More details of the interface are in the documentation. Current plan: another draft of the documentation, then a release candidate, and, if that's OK, then release 2.002000, thanks, jeffrey kegler On Tue Feb 03 17:56:04 2009, user42@zip.com.au wrote: Show quoted text

> "Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes:

> > > > a constructor might take a permanent object and add objects to it.

> > That may be outside the scope of what can easily be tested. I think > I've had situations both where I've hung data on another object, and > where the other object keeps it's own records of things about me. The > first is a leak if not cleaned up, the second is a matter for the other > object. But which is which would be nearly indistinguishable.

Fri Feb 27 04:30:16 2009 jkegl [...] cpan.org - Broken in 2.000000 added

Fri Feb 27 04:30:16 2009 jkegl [...] cpan.org - Fixed in 2.001_000 added

Fri Feb 27 19:35:47 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Sat, 28 Feb 2009 11:34:27 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > I named the option 'ignore' in keeping with the track/follow/ignore > nomenclature I use in the 2.000000 documentation. More details of the > interface are in the documentation.

In the docs for ignore you might start with what it's good for, and then go on to what it's not good for :-). I think the best thing about an ignore is it doesn't descend into the unwanted objects -- which is almost essential for the quite likely case that they have random array etc sub-parts not easily identifiable in post-filtering. As a suggestion, how about also an option like ignore_classes => [ 'My::PrintSpoolerSingleton', 'My::PermaType' ] which would cover basic blessed()+isa() ignoring. Or a single "ingore_class" if you might have just one to ignore, or accept either a string name or arrayref in one option. For symmetry the ignore option could take an array of coderefs to apply too, ignore => [ \&is_a_global, \&is_a_persistent_test_bit, \&is_a_coderef_from_a_global_func ] Incidentally, not sure about the check_ignore() feature. Maybe an example of the type of mistake it keeps you safe from would be clearer. If an ignore is just a predicate func you'd have to do something pretty bad to upset the leaks mechanism would you?

Sat Feb 28 00:16:27 2009 jkegl [...] cpan.org - Correspondence added

Show quoted text

> In the docs for ignore you might start with what it's good for, and then > go on to what it's not good for :-).

Adam Kennedy somehow found the time some months ago to guide me on writing CPAN documentation, and one of the things he pointed out is it's important to focus on readers who are skimming CPAN hastily -- someone who's in a hurry and exploring CPAN looking to save himself effort. A skimmer is always asking, "Do I care?". If the answer is, "No, you don't", that fact belongs first. For my stuff, you are a careful reader (and thank you for that), but I suspect you also do a lot of skimming in CPAN and other places, so you may know what I mean.

Sat Feb 28 00:24:02 2009 jkegl [...] cpan.org - Correspondence added

I thought about the possibility of other, more specific, ignore filters. These would have the major advantage of being safer and (probably also) more efficient than filtering with a closure. But I don't think we're 100% clear yet what are the most useful filters, and I thought it best to start with the most general option. Once we start on more specific filters than it's "What about Boolean combinations", etc. I kn ow on my other module (Parse::Marpa) I've spent days on features I've discovered to be useless clutter and backed out. So, I'm open to these possibilities, but in the short run I want to think and accumulate experience. On Fri Feb 27 19:35:47 2009, user42@zip.com.au wrote: Show quoted text

> "Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes:

> > > > I named the option 'ignore' in keeping with the track/follow/ignore > > nomenclature I use in the 2.000000 documentation. More details of the > > interface are in the documentation.

> > In the docs for ignore you might start with what it's good for, and then > go on to what it's not good for :-). > > I think the best thing about an ignore is it doesn't descend into the > unwanted objects -- which is almost essential for the quite likely case > that they have random array etc sub-parts not easily identifiable in > post-filtering. > > > As a suggestion, how about also an option like > > ignore_classes => [ 'My::PrintSpoolerSingleton', > 'My::PermaType' ] > > which would cover basic blessed()+isa() ignoring. Or a single > "ingore_class" if you might have just one to ignore, or accept either a > string name or arrayref in one option. > > For symmetry the ignore option could take an array of coderefs to apply > too, > > ignore => [ \&is_a_global, > \&is_a_persistent_test_bit, > \&is_a_coderef_from_a_global_func ] > > > Incidentally, not sure about the check_ignore() feature. Maybe an > example of the type of mistake it keeps you safe from would be clearer. > If an ignore is just a predicate func you'd have to do something pretty > bad to upset the leaks mechanism would you?

Sat Feb 28 00:35:42 2009 jkegl [...] cpan.org - Correspondence added

As for check_ignore: The t/ignore.t file in the test suite simulates some of the possible screw-ups, and tests that check_ignore actually catches them. I was pleased to discover that it is harder to screw up from inside a callback than I feared -- in particular if you follow the "best practice" of immediately copying the callback's args, you need to work a bit to screw things up. That's the good news. The bad news is that if you do screw up, perhaps 50% of the time the symptom is some mysterious infinite loop, caused by the fact you've confused Test::Weaken about which objects it has already visited and which it has not. Debugging the callbacks even with the wrapper isn't all that easy, so it's one of those areas of program where the ancient art of desk-checking becomes relevant again. Summing up: the chances of a screw-up are lower than I feared, but the consequences are so bad that I felt the need to provide check_ignore. If I got bug reports about the callbacks I'd need to create something like it anyway. Best to be pro-active and give it to the user, I figured. On Fri Feb 27 19:35:47 2009, user42@zip.com.au wrote: Show quoted text

> Incidentally, not sure about the check_ignore() feature. Maybe an > example of the type of mistake it keeps you safe from would be clearer. > If an ignore is just a predicate func you'd have to do something pretty > bad to upset the leaks mechanism would you?

Sun Mar 01 18:23:56 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Mon, 02 Mar 2009 10:21:56 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > For my stuff, you are a careful reader

Actually I'm a skimmer really :-). Too much words and I start to glaze over. Which is why I like fragments of code, or itemized lists, etc. :) For "Do I care?", I think, in general, a typical or normal use first would tell me if I'm interested to read more (about pros and cons or whatnot).

Sun Mar 01 18:35:08 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Mon, 02 Mar 2009 10:33:26 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > I was pleased to discover that it is harder to screw up from inside a > callback than I feared -- in particular if you follow the "best > practice" of immediately copying the callback's args,

Maybe you could protect internals by copying for the callback, eg. my $arg = $self->{'some_private_stuff}; &$callback ($arg); If the caller stores to $_[0] it only hurts $arg not $self (if I'm not mistaken). There might even be an advantage to do it that way, if the arg is then a strong ref where internals are only weak, or if the weakness of the original ref can be propagated to the callback ... or something like that.

Sun Mar 01 20:40:40 2009 jkegl [...] cpan.org - Correspondence added

On Sun Mar 01 18:35:08 2009, user42@zip.com.au wrote: Show quoted text

> Maybe you could protect internals by copying for the callback

That's a really good idea. I think I'll do that before the next official release. By the way, I just uploaded a new developer's release. Mainly rewrites to the docs.

Thu Mar 05 21:26:49 2009 jkegl [...] cpan.org - Correspondence added

I've just uploaded developer's release 2.001_002. It incorporates your suggestion of passing the ignore callback a copy of the probe reference, rather than the real thing. Since this greatly eases the task the check_ignore static method was designed for, I redesigned check_ignore. It now by default does a deep comparison of the two objects, a much more time-consuming check, but also a much more thorough one. It no longer takes an error callback argument -- I decided that if you wanted to get that complicated, you were better off copying check_ignore and hacking it -- that would be simpler. Instead check_ignore takes three new optional arguments. They are a maximum error count (to allow multiple error reports but also cut off infinite loops), a maximum depth for the deep comparison, and a separate maximum depth for reporting differences. There's a first draft of the documentation with details. thanks, jeffrey kegler

Fri Mar 06 16:55:43 2009 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #42503] suggest skipping "permanent" objects
Date:	Sat, 07 Mar 2009 08:54:05 +1100
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > It incorporates your suggestion of passing the ignore callback a copy of > the probe reference, rather than the real thing.

In normal circumstances of course anyone modifying their args deserves what they get. I suppose it's nice if tools that are supposed to hunt down badness protect themselves a bit. :-) Show quoted text

> Since this greatly eases the task the check_ignore static method was > designed for, I redesigned check_ignore. It now by default does a deep > comparison of the two objects, a much more time-consuming check, but > also a much more thorough one.

I'm not sure I'm convinced. If it were mine I don't think I'd worry about it initially ... or leave it in an example script to be copied by anyone having strange trouble.

Sat Mar 07 15:08:14 2009 jkegl [...] cpan.org - Correspondence added

I cranked up the thoroughness of check_ignore because after I incorporated your suggestion of pre-copying the arguments to the ignore option, almost all of the errors that check_ignore was testing for were not potential problems any more -- check_ignore in its original form had been made nearly useless. So it was either eliminate check_ignore, clutter the documentation with an almost useless method, or crank up its power. I chose the last. The fact that pre-copying elminates most bugs means the users of Test::Weaken::check_ignore are now a smaller and more desperate class of the debugging community, and will want and expect a pretty thorough check. Looked at another way, I thought it might violate the Principle of Least Surprise if the user could try out check_ignore and by default easily get false positives due to lack of depth in the comparison. Best to start out thorough and let the user back things off if that's too much. The depth of checking can be easily lowered. One of the arguments to check_ignore is a limit on the depth to which the before and after objects will be compared. Since the base pointer counts in the depth, setting the compare depth to 1, in effect, turns that check off. As you suggest, I'm not too worried about the interface to check_ignore, because it's not tied in with the rest of the package and is very easy to pull out of the source and hack up any way the user wants. On Fri Mar 06 16:55:43 2009, user42@zip.com.au wrote: Show quoted text

> "Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes:

> > Since this greatly eases the task the check_ignore static method was > > designed for, I redesigned check_ignore. It now by default does a deep > > comparison of the two objects, a much more time-consuming check, but > > also a much more thorough one.

> > I'm not sure I'm convinced. If it were mine I don't think I'd worry > about it initially ... or leave it in an example script to be copied by > anyone having strange trouble.

Wed Mar 11 00:41:24 2009 jkegl [...] cpan.org - Correspondence added

I've just released 2.001_004 to CPAN. It address this issue in this ticket. It's a release candidate, and if cpantesters blesses and all looks good, it will become 2.002000.

Sat Mar 14 13:23:57 2009 jkegl [...] cpan.org - Fixed in 2.002000 added

Sat Mar 14 13:23:57 2009 jkegl [...] cpan.org - Fixed in 2.001_000 deleted

Sat Mar 14 13:26:12 2009 jkegl [...] cpan.org - Correspondence added

The fix is in 2.002000, which I've just released on CPAN. If all looks good, in a few days I will mark this bug resolved.

Tue Mar 17 21:32:56 2009 jkegl [...] cpan.org - Correspondence added

Cpantesters looks fine. Resolved with official release 2.002000.

Tue Mar 17 21:32:57 2009 jkegl [...] cpan.org - Status changed from 'open' to 'resolved'