Bug fix for storing shared objects in shared structures

The attached patches fix this bug.  Thank to Dean Arnold for
the
suggestion to use a PL_ function pointer.


Jerry D. Hedden wrote:
> I'm trying to come up with a fix for the bug related
to
> storing shared objects inside of shared structures. 
The bug
> is that when any proxy objects for the shared object
are
> destroyed, the object's DESTROY routine is called even
> thought the object itself should not yet be destroyed.
>
> The following elicits the bug:
> -----
> #!/usr/bin/perl
>
> use strict;
> use warnings;
>
> use threads;
> use threads::shared;
>
> package Jar; {
>     my jar :shared;
>
>     sub new {
>         bless(&threads::shared::share({}), shift);
>     }
>
>     sub store {
>         my ($self, $cookie) = _;
>         push(jar, $cookie);
>         print("JAR   : Cookie storedn");
>         return $jar[-1];        # BUG: The cookie is
destroyed here
>     }
> }
>
> package Cookie; {
>     my $destruction_count = 0;
>
>     sub new {
>         bless(&threads::shared::share({}), shift);
>     }
>
>     sub DESTROY {
>         $destruction_count++;
>         print("COOKIE: destruction count =
$destruction_countn");
>     }
> }
>
> package main;
>
> MAIN:
> {
>     my $jar = Jar->new();
>     my $cookie = Cookie->new();
>
>     print("MAIN  : Storing cookien");
>     $jar->store($cookie);
>
>     print("nMAIN  : Cookie should not have been
destroyed yetn");
>
>     print("nMAIN  : Exiting scopen")
> }
>
> print("nDONEn");
> -----
> The above outputs:
>     MAIN  : Storing cookie
>     JAR   : Cookie stored
>     COOKIE: destruction count = 1
>
>     MAIN  : Cookie should not have been destroyed yet
>
>     MAIN  : Exiting scope
>     COOKIE: destruction count = 2
>
>     DONE
>
> which shows that DESTROY is called twice - the first
time by
> the destruction of a proxy object.
>
> I am attempting to fix this bug by first providing a
call in
> threads::shared (ext/threads/shared/shared.xs) to
report on
> whether or not a shared object should be destroyed:  If
the
> ref is shared, and its refcnt is greater than one, then
it
> should NOT be destroyed.

On 06/11/2007, Jerry D. Hedden <jdheddencpan.org> wrote:
> The attached patches fix this bug.  Thank to Dean
Arnold for the
> suggestion to use a PL_ function pointer.

OK. The blead.patch can go in without problems: it does
nothing and
prepares binary compatibilty for upcoming 5.10.x and
upcoming CPAN
releases of threads::shared. But the shared.patch itself
might be a
bit dangerous to put in bleadperl this late, knowing that it
could
always be upgraded from CPAN ? What do you think ?

On 11/6/07, Rafael Garcia-Suarez <rgarciasuarezgmail.com> wrote:
> On 06/11/2007, Jerry D. Hedden <jdheddencpan.org> wrote:
> > The attached patches fix this bug.  Thank to Dean
Arnold for the
> > suggestion to use a PL_ function pointer.
>
> OK. The blead.patch can go in without problems: it does
nothing and
> prepares binary compatibilty for upcoming 5.10.x and
upcoming CPAN
> releases of threads::shared. But the shared.patch
itself might be a
> bit dangerous to put in bleadperl this late, knowing
that it could
> always be upgraded from CPAN ? What do you think ?

I plan to release the updated threads::shared to CPAN if
you
incorporate these patches into blead.  However, there is no
compatibility issues:  the two patches will work with or
without each
other.  You can see this by noting the #ifdef PL_destroyhook
in
shared.xs.

On 11/6/07, Jerry D. Hedden <jdheddencpan.org> wrote:
> On 11/6/07, Rafael Garcia-Suarez <rgarciasuarezgmail.com> wrote:
> > On 06/11/2007, Jerry D. Hedden <jdheddencpan.org> wrote:
> > > The attached patches fix this bug.  Thank to
Dean Arnold for the
> > > suggestion to use a PL_ function pointer.
> >
> > OK. The blead.patch can go in without problems: it
does nothing and
> > prepares binary compatibilty for upcoming 5.10.x
and upcoming CPAN
> > releases of threads::shared. But the shared.patch
itself might be a
> > bit dangerous to put in bleadperl this late,
knowing that it could
> > always be upgraded from CPAN ? What do you think
?
>
> I plan to release the updated threads::shared to CPAN
if you
> incorporate these patches into blead.  However, there
is no
> compatibility issues:  the two patches will work with
or without each
> other.  You can see this by noting the #ifdef
PL_destroyhook in
> shared.xs.

In fact, I just uploaded threads::shared 1.15 to CPAN.  

Jerry D. Hedden wrote:
> The attached patches fix this bug.  Thank to Dean
Arnold for the
> suggestion to use a PL_ function pointer.
> 

Just to double check: does this permit *any*
blessed, tied SV to make use of this feature
(assuming it's tie module chains the PL_destroyhook) ?
Or only threads::shared ? Or only if ithreads
is configured ? My read of the code indicates it does,
which means I can borrow it for Thread::Sociable...and
perhaps DBI (which has blessed, tied hashes) might as well
?

Which also reminds me of another threads::shared issue:
any chance of threads::shared chaining the lockhook
and destroyhook (ie, saving the current funcptr before
assigning to it, then checking the SV is shared
when its hook functions get called, and forwarding on
if the SV isn't shared ? (I've been meaning to put together
a patch for this). It may not be needed
if CORE always sets up threads::shared 1st, but just
in case some other module wants to hook in, and we don't
know the hooking order beforehand, it might be useful.
(Note this doesn't require CORE changes, just
threads::shared
changes)

Dean Arnold