xslate / p5-Mouse

Lightweight class builder for Perl, as a subset of Moose
https://metacpan.org/release/Mouse
Other
46 stars 32 forks source link

Fix for https://github.com/gfx/p5-Mouse/issues/88 #89

Closed sergeykolychev closed 3 years ago

sergeykolychev commented 5 years ago

The changes: Making sure that applying a role at runtime to an instance properly checks for the existence of the role's required attributes in that instance.

Hi there, I'm long time user of the module (thanks guys), but never yet contributed. This is one possible solution to the problem reported in the issue. I obviously don't know any history behind the 'is_cloning' and not really sure if this issue warrants this change. But if the issue important, here's the change. I made sure that all tests pass and added a new test for the fix.

sergeykolychev commented 5 years ago

@gfx @skaji Hi there, Could you please see if this pull makes sense ? Thanks.

skaji commented 5 years ago

@sergeykolychev Thanks for PR.

It seems that this PR introduces a memory leak.

#!/usr/bin/env perl
use strict;
use warnings;

package MyRole;

use Mouse::Role;

has 'name' =>
(
    is       => 'ro',
    isa      => 'Str',
    required => 1
);

no Mouse::Role;

package MyClass;

use Mouse;
use Mouse::Util;

sub BUILD
{
    my $self = shift;
    my $args = shift;

    Mouse::Util::apply_all_roles($self, 'MyRole');
}

__PACKAGE__->meta->make_immutable;

package main;

warn "-> top -p $$\n";
for (1..1000) {
    warn "loop $_\n";
    for (1..1000) {
        my $obj = eval { MyClass->new() };
    }
    sleep 1;
}

1;
sergeykolychev commented 5 years ago

@skaji Why would you wrap it in eval and let it fail and accumulate cruft 1000 iterations ? The only thing this PR introduces is an input verification, and the program is supposed to exit with an error. The reason for the leak is that there's this loop: 1) create object 2) verify input 3) die If you have it in eval the objects created in 1) are never cleared. And please try your code with master version of Mouse, your code will cause current Mouse leak as well.

sergeykolychev commented 5 years ago

@skaji Just to reiterate, the memory leak is preexisting and not related to this PR. I'll investigate the reason for this leak when I have time, but it should not be used against this PR. I'd accept if you deem the PR unnecessary (this bug was in Mouse for 8 years now, and can remain for next 8 I guess).

potatogim commented 5 years ago

Could you let me know how are this PR going?