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

[Robert Stone]

Greetings,

Tried in 5.16.3 and 5.24.0.  One of our debugging steps was to take
advantage of perlbrew and try it in a different version of perl.  Same
result.

Interesting idea that this could be a perl bug.  I don't know if I'd call
this a bug though, as much as perl doing *exactly* what you asked it to
without knowing what you were asking.

Best Regards,
Robert Stone

On Fri, Dec 16, 2016 at 4:40 PM, Julian Brown via Houston <houston at pm.org>
wrote:

> First of I love Test::MockModule,
>
> I am wondering if you run this on different versions of Perl if your
> results would be different and possibly a bug in Perl for that version?
>
> Julian
>
> On Fri, Dec 16, 2016 at 12:43 PM, Robert Stone via Houston <houston at pm.org
> > wrote:
>
>> 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
>>
>> _______________________________________________
>> Houston mailing list
>> Houston at pm.org
>> http://mail.pm.org/mailman/listinfo/houston
>> Website: http://houston.pm.org/
>>
>
>
> _______________________________________________
> Houston mailing list
> Houston at pm.org
> http://mail.pm.org/mailman/listinfo/houston
> Website: http://houston.pm.org/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.pm.org/pipermail/houston/attachments/20161216/c04d05ad/attachment-0001.html>