Fix intermittent failures in the LeakTracer on 5.18+, remove all workarounds

ARGH! In the end the issue turned out to be stupid-simple: when PadWalker is
called it always returns us a hashref. This hashref is traversed, with its
own address recorded as seen. Once processed, the hashref itself is GCed,
leaving a stale entry in $seen_refs.

Up until 5.18 this was not a problem, likely due to no address reuse. However
after 5.18 PadWalker is very happy to return different hashrefs under the same
address over and over again. The $seen_refs entry leads to a skip, and the
actual contents of the pad are never examined - we've seen this hash already.

ARGH!

The decisive change is the single line around PadWalker - everything else is
cleanup of workarounds for this problem.

(cherry pick of 1a77219a)

diff --git a/Changes b/Changes
index b3ae1cb..36dfaeb 100644 (file)
--- a/Changes
+++ b/Changes
@@ -2,6 +2,7 @@ Revision history for DBIx::Class
 
     * Misc
         - Depend on newer Moo, to benefit from a safer runtime (RT#93004)
+        - Fix intermittent failures in the LeakTracer on 5.18+
 
 0.082810 2014-10-25 13:58 (UTC)
     * Fixes

diff --git a/t/52leaks.t b/t/52leaks.t
index fdd0d52..0a907ef 100644 (file)
--- a/t/52leaks.t
+++ b/t/52leaks.t
@@ -320,9 +320,6 @@ unless (DBICTest::RunMode->is_plain) {
       ! DBICTest::RunMode->is_plain
         and
       ! $ENV{DBICTEST_IN_PERSISTENT_ENV}
-        and
-      # FIXME - investigate wtf is going on with 5.18
-      ! ( $] > 5.017 and $ENV{DBIC_TRACE_PROFILE} )
     ) {
 
       # FIXME - ideally we should be able to just populate an alternative

diff --git a/t/lib/DBICTest/Util/LeakTracer.pm b/t/lib/DBICTest/Util/LeakTracer.pm
index 4c90362..efbe1d4 100644 (file)
--- a/t/lib/DBICTest/Util/LeakTracer.pm
+++ b/t/lib/DBICTest/Util/LeakTracer.pm
@@ -11,7 +11,6 @@ use Data::Dumper::Concise;
 use DBICTest::Util 'stacktrace';
 use constant {
   CV_TRACING => DBIx::Class::Optional::Dependencies->req_ok_for ('test_leaks_heavy'),
-  SKIP_SCALAR_REFS => ( $] > 5.017 ) ? 1 : 0,
 };
 
 use base 'Exporter';
@@ -43,10 +42,6 @@ sub populate_weakregistry {
       for keys %$reg;
   }
 
-  # FIXME/INVESTIGATE - something fishy is going on with refs to plain
-  # strings, perhaps something to do with the CoW work etc...
-  return $target if SKIP_SCALAR_REFS and reftype($target) eq 'SCALAR';
-
   if (! defined $weak_registry->{$refaddr}{weakref}) {
     $weak_registry->{$refaddr} = {
       stacktrace => stacktrace(1),
@@ -141,7 +136,7 @@ sub visit_refs {
       elsif (CV_TRACING and $type eq 'CODE') {
         $visited_cnt += visit_refs({ %$args, refs => [ map {
           ( !isweak($_) ) ? $_ : ()
-        } scalar PadWalker::closed_over($r) ] }); # scalar due to RT#92269
+        } values %{ scalar PadWalker::closed_over($r) } ] }); # scalar due to RT#92269
       }
       1;
     } or warn "Could not descend into @{[ refdesc $r ]}: $@\n";
@@ -179,8 +174,6 @@ sub symtable_referenced_addresses {
 
   my $refs_per_pkg;
 
-  my $dummy_addresslist;
-
   my $seen_refs = {};
   visit_namespaces(
     action => sub {
@@ -196,14 +189,7 @@ sub symtable_referenced_addresses {
       $refs_per_pkg->{$pkg} += visit_refs (
         seen_refs => $seen_refs,
 
-        # FIXME FIXME FIXME
-        # This is so damn odd - if we feed a constsub {1} (or in fact almost
-        # anything other than the actionsub below, any scalarref will show
-        # up as a leak, trapped by... something...
-        # Ideally we should be able to const this to sub{1} and just return
-        # $seen_refs (in fact it is identical to the dummy list at the end of
-        # a run here). Alas this doesn't seem to work, so punt for now...
-        action => sub { ++$dummy_addresslist->{ hrefaddr $_[0] } },
+        action => sub { 1 },
 
         refs => [ map { my $sym = $_;
           # *{"$pkg$sym"}{CODE} won't simply work - MRO-cached CVs are invisible there