From @nwc10

With $ENV{PERL_DESTRUCT_LEVEL} set to 2, perl should free everything​:

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full --show-reachable=yes ./perl -Ilib -e0
==10636== Memcheck, a memory error detector
==10636== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==10636== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==10636== Command​: ./perl -Ilib -e0
==10636==
==10636==
==10636== HEAP SUMMARY​:
==10636== in use at exit​: 0 bytes in 0 blocks
==10636== total heap usage​: 666 allocs, 666 frees, 177,332 bytes allocated
==10636==
==10636== All heap blocks were freed -- no leaks are possible
==10636==
==10636== For counts of detected and suppressed errors, rerun with​: -v
==10636== ERROR SUMMARY​: 0 errors from 0 contexts (suppressed​: 4 from 4)

Note the pleasing "All heap blocks were freed -- no leaks are possible".

Somewhat frustratingly, glibc's dl_open implementation has 6 still reachable
blocks​:

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full ./perl -Ilib -Mthreads -e0
==28896== Memcheck, a memory error detector
==28896== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==28896== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==28896== Command​: ./perl -Ilib -Mthreads -e0
==28896==
==28896==
==28896== HEAP SUMMARY​:
==28896== in use at exit​: 1,572 bytes in 6 blocks
==28896== total heap usage​: 6,932 allocs, 6,926 frees, 1,500,001 bytes allocated
==28896==
==28896== LEAK SUMMARY​:
==28896== definitely lost​: 0 bytes in 0 blocks
==28896== indirectly lost​: 0 bytes in 0 blocks
==28896== possibly lost​: 0 bytes in 0 blocks
==28896== still reachable​: 1,572 bytes in 6 blocks
==28896== suppressed​: 0 bytes in 0 blocks
==28896== Reachable blocks (those to which a pointer was found) are not shown.
==28896== To see them, rerun with​: --leak-check=full --show-reachable=yes
==28896==
==28896== For counts of detected and suppressed errors, rerun with​: -v
==28896== ERROR SUMMARY​: 0 errors from 0 contexts (suppressed​: 4 from 4)

so in what follows, 6 reachable objects are not our concern. But 55 blocks
and 87M are​:

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full ./perl -Ilib -Mthreads -Mthreads​::shared -e0
==4378== Memcheck, a memory error detector
==4378== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==4378== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==4378== Command​: ./perl -Ilib -Mthreads -Mthreads​::shared -e0
==4378==
==4378==
==4378== HEAP SUMMARY​:
==4378== in use at exit​: 88,775 bytes in 61 blocks
==4378== total heap usage​: 8,818 allocs, 8,757 frees, 2,046,301 bytes allocated
==4378==
==4378== LEAK SUMMARY​:
==4378== definitely lost​: 0 bytes in 0 blocks
==4378== indirectly lost​: 0 bytes in 0 blocks
==4378== possibly lost​: 0 bytes in 0 blocks
==4378== still reachable​: 88,775 bytes in 61 blocks
==4378== suppressed​: 0 bytes in 0 blocks
==4378== Reachable blocks (those to which a pointer was found) are not shown.
==4378== To see them, rerun with​: --leak-check=full --show-reachable=yes
==4378==
==4378== For counts of detected and suppressed errors, rerun with​: -v
==4378== ERROR SUMMARY​: 0 errors from 0 contexts (suppressed​: 4 from 4)

With --show-reachable=yes the leaks all look like this​:

==17136== 26 bytes in 1 blocks are still reachable in loss record 1 of 51
==17136== at 0x4C25D8C​: malloc (vg_replace_malloc.c​:270)
==17136== by 0x546226​: Perl_safesysmalloc (util.c​:90)
==17136== by 0x5488BC​: Perl_savepv (util.c​:929)
==17136== by 0x71E083​: Perl_new_collate (locale.c​:228)
==17136== by 0x71FE8A​: Perl_init_i18nl10n (locale.c​:501)
==17136== by 0x45859B​: perl_construct (perl.c​:264)
==17136== by 0x66B59FF​: Perl_sharedsv_init (shared.xs​:1252)
==17136== by 0x66BEA25​: boot_threads__shared (shared.xs​:1715)
==17136== by 0x5AF1C3​: Perl_pp_entersub (pp_hot.c​:2743)
==17136== by 0x545852​: Perl_runops_debug (dump.c​:2258)
==17136== by 0x460463​: Perl_call_sv (perl.c​:2816)
==17136== by 0x46B7DB​: Perl_call_list (perl.c​:4898)

And of those 45 loss records that are from threads​::shared​:

$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full --show-reachable=yes ./perl -Ilib -Mthreads -Mthreads​::shared -e0 2>&1 | fgrep -c 'Perl_sharedsv_init (shared.xs​:1252)'
44
$ PERL_DESTRUCT_LEVEL=2 valgrind --leak-check=full --show-reachable=yes ./perl -Ilib -Mthreads -Mthreads​::shared -e0 2>&1 | fgrep -c 'Perl_sharedsv_init (shared.xs​:1251)'
1

The lines in question being the last two of​:

void
Perl_sharedsv_init(pTHX)
{
  dTHXc;
  /* This pair leaves us in shared context ... */
  PL_sharedsv_space = perl_alloc();
  perl_construct(PL_sharedsv_space);

I'm not sure what the best solution is. One way would be to move
PL_sharedsv_space from shared.xs into perlvar.h, and have perl_destruct()
be responsible for freeing it at the right time. The other way I can think of
is to have shared.xs attach magic to a global SV, and have that magic be
responsible for freeing it at the right time.

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Nicholas Clark

From perl@profvince.com

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Looks like the main thread can be identified by "PL_curinterp == aTHX".
If so, maybe the shared memory can be freed from a call_atexit()
callback that checks this condition.

Vincent

The RT System itself - Status changed from 'new' to 'open'

From @nwc10

On Mon, Aug 05, 2013 at 11​:53​:51AM -0300, Vincent Pit wrote​:

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Looks like the main thread can be identified by "PL_curinterp == aTHX".
If so, maybe the shared memory can be freed from a call_atexit()
callback that checks this condition.

Yes, using call_atexit() looks like a much better plan.

(I'm not working on fixing this bug. I'm also not in any way hinting that
Vincent should either. I just wanted to report the bug, as it seemed to be
unknown.)

Nicholas Clark

From @tsee

On 08/05/2013 04​:58 PM, Nicholas Clark wrote​:

On Mon, Aug 05, 2013 at 11​:53​:51AM -0300, Vincent Pit wrote​:

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Looks like the main thread can be identified by "PL_curinterp == aTHX".
If so, maybe the shared memory can be freed from a call_atexit()
callback that checks this condition.

Yes, using call_atexit() looks like a much better plan.

(I'm not working on fixing this bug. I'm also not in any way hinting that
Vincent should either. I just wanted to report the bug, as it seemed to be
unknown.)

Whoever ends up fixing it should also document the fix in perlxs - it's
a common pitfall about cleaning up global data in a threaded environment.

--Steffen

From @iabyn

On Mon, Aug 05, 2013 at 03​:58​:24PM +0100, Nicholas Clark wrote​:

On Mon, Aug 05, 2013 at 11​:53​:51AM -0300, Vincent Pit wrote​:

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Looks like the main thread can be identified by "PL_curinterp == aTHX".
If so, maybe the shared memory can be freed from a call_atexit()
callback that checks this condition.

Yes, using call_atexit() looks like a much better plan.

I don't think that will work. Consider an embedded environment like Apache
and mod_perl.

This can create multiple perl interpreters A, B, C, ... and potentially
run them in parallel within different (Apache) threads.

Interpreter A may run some code that calls threads->new(), creating new
interpreters A1, A2, ..., running in different (perl) threads. The code may
also use threads​::shared, and thus create a shared interpreter, AS.

Similarly, B may create BS, B1, B2, ...; C may create CS, C1, C2, ..., etc.

So you end up with multiple pools of interpreters. Clearly when all of
A,A1,A2,... finish, you need to be sure to free AS. This may happen before
or after all the B's finish, and certainly well before the process as a
whole is exiting.

Note that I'm not clear in a mod_perl-style environment, what happens
(and should happen) if thread A finishes before threads A1, A2 etc have
finished. In a normal perl setup you get the warning about threads still
running, then the process exits.

--
Now is the discount of our winter tent
  -- sign seen outside camping shop

From @nwc10

On Mon, Aug 12, 2013 at 05​:09​:21PM +0100, Dave Mitchell wrote​:

On Mon, Aug 05, 2013 at 03​:58​:24PM +0100, Nicholas Clark wrote​:

On Mon, Aug 05, 2013 at 11​:53​:51AM -0300, Vincent Pit wrote​:

Am I correct in thinking that under ithreads PL_curinterp is NULL only in
the topmost parent thread? If so, the "right time" is when the C code is
called and PL_curinterp is NULL. Yes, this is only a 90% solution, as it will
only clean up properly if threads​::shared was loaded by the top level thread,
but it's 90% better than what we have at the moment.

Looks like the main thread can be identified by "PL_curinterp == aTHX".
If so, maybe the shared memory can be freed from a call_atexit()
callback that checks this condition.

Yes, using call_atexit() looks like a much better plan.

I don't think that will work. Consider an embedded environment like Apache
and mod_perl.

This can create multiple perl interpreters A, B, C, ... and potentially
run them in parallel within different (Apache) threads.

Interpreter A may run some code that calls threads->new(), creating new
interpreters A1, A2, ..., running in different (perl) threads. The code may
also use threads​::shared, and thus create a shared interpreter, AS.

Similarly, B may create BS, B1, B2, ...; C may create CS, C1, C2, ..., etc.

So you end up with multiple pools of interpreters. Clearly when all of
A,A1,A2,... finish, you need to be sure to free AS. This may happen before
or after all the B's finish, and certainly well before the process as a
whole is exiting.

Note that I'm not clear in a mod_perl-style environment, what happens
(and should happen) if thread A finishes before threads A1, A2 etc have
finished. In a normal perl setup you get the warning about threads still
running, then the process exits.

I think that it all still works. call_atexit() is Perl_call_atexit(), not
the confusingly similarly named C library function. So​:

void
Perl_call_atexit(pTHX_ ATEXIT_t fn, void *ptr)
{
  dVAR;
  Renew(PL_exitlist, PL_exitlistlen+1, PerlExitListEntry);
  PL_exitlist[PL_exitlistlen].fn = fn;
  PL_exitlist[PL_exitlistlen].ptr = ptr;
  ++PL_exitlistlen;
}

which get called at this point in global destruction​:

  sv_clean_objs();

  /* unhook hooks which will soon be, or use, destroyed data */
  SvREFCNT_dec(PL_warnhook);
  PL_warnhook = NULL;
  SvREFCNT_dec(PL_diehook);
  PL_diehook = NULL;

  /* call exit list functions */
  while (PL_exitlistlen-- > 0)
  PL_exitlist[PL_exitlistlen].fn(aTHX_ PL_exitlist[PL_exitlistlen].ptr);

  Safefree(PL_exitlist);

  PL_exitlist = NULL;
  PL_exitlistlen = 0;

  SvREFCNT_dec(PL_registered_mros);

and so combine that with the (PL_curinterp == aTHX) test and you can arrange
that only A (and B and C) clear up their respective shared pools.

If A finishes before A1 (or any of the others) PL_threadhook() already returns
1, which vetos any destruction. That happens a lot earlier​:

  if (PL_threadhook(aTHX)) {
  /* Threads hook has vetoed further cleanup */
  PL_veto_cleanup = TRUE;
  return STATUS_EXIT;
  }

I didn't realise that the code is designed to leak. In that​:

$ perl -Mthreads -e 'sub DESTROY {warn "I did get called"}; $o = bless [];'
I did get called at -e line 1 during global destruction.

versus​:

$ perl -Mthreads -e 'sub DESTROY {warn "I did get called"}; $o = bless []; threads->new(sub {sleep 2})'
Perl exited with active threads​:
  1 running and unjoined
  0 finished and unjoined
  0 running and detached

ie global destruction does not run. It's clearly an error (and the loud
message tries to say this), but it does mean that an implementation detail
of a module could subvert a program which loads it. Something like this​:

$ cat Sleeper.pm
#!perl
use warnings;
use threads;

package Sleeper;

sub new {
  my $o = threads->new(sub { sleep 3; });
  bless \$o;
}

sub DESTROY {
  ${$_[0]}->join();
}

1;

__END__

Where putting one of those objects into a lexical is fine, but if it happens
to end up referenced by a global variable, action at a distance​:

$ perl -MSleeper -e 'sub DESTROY {warn "I did get called in " . threads->tid}; $o = bless []; my $a = Sleeper->new'
I did get called in 1 at -e line 1 during global destruction.
I did get called in 0 at -e line 1 during global destruction.
$ perl -MSleeper -e 'sub DESTROY {warn "I did get called in " . threads->tid}; $o = bless []; our $a = Sleeper->new'
Perl exited with active threads​:
  1 running and unjoined
  0 finished and unjoined
  0 running and detached

I don't have an answer for this particular digression.

But I think we can clear up shared space correctly in an embedded environment
if we are already clearing up everything else correctly (ie no veto).

Nicholas Clark

From @iabyn

On Tue, Aug 20, 2013 at 02​:27​:06PM +0100, Nicholas Clark wrote​:

I think that it all still works. call_atexit() is Perl_call_atexit(), not
the confusingly similarly named C library function. So​:

Ah, I was indeed confusing the two.

I don't have an answer for this particular digression.

Me neither.

But I think we can clear up shared space correctly in an embedded
environment if we are already clearing up everything else correctly (ie
no veto).

Oh good.

--
Nothing ventured, nothing lost.