Bug #109139 for Moo: older Sub::Name leaks memory

Thu Nov 19 08:37:28 2015 develop [...] traveljury.com - Ticket created

Moo doesn't specify a minimum version for Sub::Name. Sub::Name 0.07 has a memory leak (see https://metacpan.org/changes/distribution/Sub-Name#L27) which is fixed in 0.08. Upgrading Moo doesn't fix this as it thinks 0.07 is an acceptable version. Found via https://github.com/elastic/elasticsearch-perl/issues/45

Thu Nov 19 12:29:47 2015 haarg [...] haarg.org - Correspondence added

Moo doesn't specify a dependency on Sub::Name at all. We could add it conditionally on the availability of a compiler, but I'd rather avoid that if possible. It seems odd that this would cause significant issues. Sub::Name is only used in a limited number of places when generating classes. Is your codebase generating new classes often?

Thu Nov 19 12:29:49 2015 The RT System itself - Status changed from 'new' to 'open'

Thu Nov 19 13:18:23 2015 ether [...] cpan.org - Subject changed from (no value) to 'older Sub::Name leaks memory'

Thu Nov 19 13:31:43 2015 ether [...] cpan.org - Correspondence added

On 2015-11-19 09:29:47, haarg wrote: Show quoted text

> Moo doesn't specify a dependency on Sub::Name at all. We could add it > conditionally on the availability of a compiler, but I'd rather avoid > that if possible.

We could do something similar to what JSON::MaybeXS does with JSON::XS: if it is already installed, make sure that it is a recent version. I've proposed a patch in https://github.com/moose/Moo/pull/22.

Thu Nov 19 14:02:45 2015 develop [...] traveljury.com - Correspondence added

On Thu Nov 19 12:29:47 2015, haarg wrote: Show quoted text

> Moo doesn't specify a dependency on Sub::Name at all. We could add it > conditionally on the availability of a compiler, but I'd rather avoid > that if possible. > > It seems odd that this would cause significant issues. Sub::Name is > only used in a limited number of places when generating classes. Is > your codebase generating new classes often?

That's a very good question. I pulled out NYTProf and it turns out that subname() is actually being called frequently by Try::Tiny: https://metacpan.org/source/DOY/Try-Tiny-0.22/lib/Try/Tiny.pm#L60 Would you prefer me to move this ticket there?

Thu Nov 19 14:06:03 2015 ether [...] cpan.org - Correspondence added

On 2015-11-19 11:02:45, DRTECH wrote: Show quoted text

> On Thu Nov 19 12:29:47 2015, haarg wrote:

> > Moo doesn't specify a dependency on Sub::Name at all. We could add > > it > > conditionally on the availability of a compiler, but I'd rather avoid > > that if possible. > > > > It seems odd that this would cause significant issues. Sub::Name is > > only used in a limited number of places when generating classes. Is > > your codebase generating new classes often?

> > That's a very good question. I pulled out NYTProf and it turns out > that subname() is actually being called frequently by Try::Tiny: > https://metacpan.org/source/DOY/Try-Tiny-0.22/lib/Try/Tiny.pm#L60 > > Would you prefer me to move this ticket there?

I think at a minimum if we do something in Moo's Makefile.PL to upgrade Sub::Name, we should do the same thing in Try::Tiny -- since it also only uses Sub::Name optionally.

Thu Nov 19 14:08:20 2015 ether [...] cpan.org - Correspondence added

Perhaps instead of doing backflips in Makefile.PL, lib/Moo/_Utils.pm should only use Sub::Name if it's a new enough version (but keep the version bump in the runtime-recommends section). We could just add a ->VERSION('0.08') call to the existing eval.

Thu Nov 19 14:18:10 2015 haarg [...] haarg.org - Correspondence added

Naming some of the subs is important for interoperability with Moose, and the number of subs we name is rather small. I'm inclined to not change anything in Moo. For Try::Tiny, just having it not use Sub::Name < 0.08 seems viable.

Sat Dec 05 22:09:34 2015 haarg [...] haarg.org - Correspondence added

I've updated Moo's recommendation of Sub::Name to be 0.08. Moo doesn't use Sub::Name enough for the memory leak to be a big concern - it's only done at class setup time. Fixed in repo. https://github.com/moose/Moo/commit/6f59d3e904e91cb4ba9a8a7e4b0cfe9cfdf55c7c

Sat Dec 05 22:09:42 2015 haarg [...] haarg.org - Status changed from 'open' to 'patched'

Mon Feb 29 17:49:11 2016 haarg [...] haarg.org - Correspondence added

Resolved in Moo 2.001000

Mon Feb 29 17:49:18 2016 haarg [...] haarg.org - Status changed from 'patched' to 'resolved'