[pm-h] Adventures In Getting Bitten By a Closure - Is there a better way to address this issue?

[Robert Stone]

Greetings,


tl;dr - refcount unexpectedly incremented, caused DESTROY method on object
to never get called.  Used a workaround to address but perhaps there is a
cleaner way?


Jocelyn and I just spent about 3 hours tracking down why a DESTROY wasn't
being called, a whole bunch of refcounts later we found the problem.  I'm
only fairly confident in my analysis below and would love it for any of you
to chime in and say "Yep!  Sounds about right" or "No way, what actually
happened was..."  Should be good times for all of us.

For those of you who aren't fully versed on the magic that is
Test::MockModule, it's the greatest thing since the circumflexion
operator.  It allows you to mock a method of an object and then when the
variable that you assigned Test::MockModule->new to goes out of scope then
the mock is removed.  In general, very handy and it works perfectly.

Here is the subroutine I was using to define and install my mock:

  sub restrict_search_space {
      my ( $user_ids ) = pos_validated_list( \@_, { isa => 'ArrayRef[Int]'
} );

      my $mocked_user = Test::MockModule->new( 'FreeChat::API::Model::User'
);
      $mocked_user->mock(
          'search',
          sub {
              my $self       = shift;
              my $condition  = shift;
              my $attributes = shift // {};

              $condition->{'me.id'} = { 'in' => $user_ids };
              return $mocked_user->original( 'search' )->( $self,
$condition, $attributes );
          } );

      return $mocked_user;
  }

Seems easy enough right?  I create an instance of Test::MockModule that
mocks FreeChat::API::Model::User.  Then I mock the search method so that I
can restrict the search space by adding an additional condition to the
WHERE clause that is ultimately passed down to DBIx's search method.
Because what I really want here is an AROUND instead of a straight up mock
I end up having to call $mocked_user->original('search') to get the
original unmocked method, thereby making this into a sort of AROUND.

This works no problem until you try to do something like this (actual
tests!  Names were not changed to protect the guilty):

      subtest 'Gender' => sub {
          my ( $mech, $user ) = get_mech_and_user();

          my $male_user   = create_user( gender => 'Male' );
          my $female_user = create_user( gender => 'Female' );

          my $mocked_user = restrict_search_space( [ $male_user->id,
$female_user->id ] );

          $mech->get_ok( '/users?gender=Male' );

          note( 'Raw JSON Response: ' . $mech->content );
          my $json = from_json( $mech->content );

          ok( ( grep { $_->{id} == $male_user->id } @{$json} ), 'Matching
user correctly identified' );
          ok( !( grep { $_->{id} == $female_user->id } @{$json} ),
'Unmatching user correctly not identified' );
      };

      subtest 'Status' => sub {
          my ( $mech, $user ) = get_mech_and_user();

          my $active_user  = create_user( status => 'Active' );
          my $offline_user = create_user( status => 'Offline' );

          my $mocked_user = restrict_search_space( [ $active_user->id,
$offline_user->id ] );

          $mech->get_ok( '/users?status=Active' );

          note( 'Raw JSON Response: ' . $mech->content );
          my $json = from_json( $mech->content );

          cmp_ok( $mech->status, '==', 200, 'Correct status' );

          ok( ( grep { $_->{id} == $active_user->id } @{$json} ), 'Matching
user correctly identified' );
          ok( !( grep { $_->{id} == $offline_user->id } @{$json} ),
'Unmatching user correctly not identified' );
      };

The first subtest passes no problem, the second one though does not.  Each
one does pass in isolation, but run together they fail.  It fails because
the instance of Test::MockModule never goes out of scope (the object
pointed to by $mocked_user has a refcount of 2, not of 1) and therefore the
mock never gets uninstalled.  CURSES!

See the issue?

      my $mocked_user = Test::MockModule->new( 'FreeChat::API::Model::User'
);
      $mocked_user->mock(
          'search',
          sub {
              my $self       = shift;
              my $condition  = shift;
              my $attributes = shift // {};

              $condition->{'me.id'} = { 'in' => $user_ids };
              *return $mocked_user->original( 'search' )->( $self,
$condition, $attributes );*
          } );

It seems that because I called $mocked_user here and have the entire thing
wrapped in a sub, I'm creating a closure causing the refcount on
$mocked_user to be incremented.  It seems that one of the references comes
from restrict_search_space's $mocked_user and the other comes from the
subtest's $mocked_user.  When I make the below modification:

  sub restrict_search_space {
      my ( $user_ids ) = pos_validated_list( \@_, { isa => 'ArrayRef[Int]'
} );

      my $mocked_user = Test::MockModule->new( 'FreeChat::API::Model::User'
);
      *my $original_method;*
      $mocked_user->mock(
          'search',
          sub {
              my $self       = shift;
              my $condition  = shift;
              my $attributes = shift // {};

              $condition->{'me.id'} = { 'in' => $user_ids };
              return *$original_method*->( $self, $condition, $attributes );
          } );

      *$original_method = $mocked_user->original( 'search' );*

      return $mocked_user;
  }

Everything worked as expected, at the end of the subtest the $mocked_user
goes out of scope and the method gets unmocked.

The real question, while this works it seems awfully hacky and ugly.  Is
there a better way to do this?  Should I have used weaken somewhere?

Just thought you all would enjoy commiserating with us.  Hope everyone is
having a safe and happy holiday.

Best Regards,
Robert Stone
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.pm.org/pipermail/houston/attachments/20161216/56c6b11e/attachment.html>