[PATCH] Bug fix for storing shared objects in shared structures
[email protected] ("Jerry D. Hedden") Tue, 6 Nov 2007 14:36:40 -0500
| Newsgroups | perl.perl5.porters,perl.ithreads |
|---|---|
| Message-ID | <[email protected]> |
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 stored\n");
> 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_count\n");
> }
> }
>
> package main;
>
> MAIN:
> {
> my $jar = Jar->new();
> my $cookie = Cookie->new();
>
> print("MAIN : Storing cookie\n");
> $jar->store($cookie);
>
> print("\nMAIN : Cookie should not have been destroyed yet\n");
>
> print("\nMAIN : Exiting scope\n")
> }
>
> print("\nDONE\n");
> -----
> 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.
blead.patch
(application/octet-stream, 3.8 KB)
--- /var/tmp/perl-current/embed.fnc 2007-11-05 09:54:57.960224400 -0500
+++ perl-current/embed.fnc 2007-11-06 10:54:15.760124700 -0500
@@ -1114,6 +1114,7 @@
Adp |void |sv_nosharing |NULLOK SV *sv
Adpbm |void |sv_nolocking |NULLOK SV *sv
+Adp |bool |sv_destroyable |NULLOK SV *sv
#ifdef NO_MATHOMS
Adpbm |void |sv_nounlocking |NULLOK SV *sv
#else
--- /var/tmp/perl-current/embedvar.h 2007-10-19 16:17:53.129568500 -0400
+++ perl-current/embedvar.h 2007-11-06 10:58:48.425178300 -0500
@@ -115,6 +115,7 @@
#define PL_defoutgv (vTHX->Idefoutgv)
#define PL_defstash (vTHX->Idefstash)
#define PL_delaymagic (vTHX->Idelaymagic)
+#define PL_destroyhook (vTHX->Idestroyhook)
#define PL_diehook (vTHX->Idiehook)
#define PL_dirty (vTHX->Idirty)
#define PL_doextract (vTHX->Idoextract)
@@ -427,6 +428,7 @@
#define PL_Idefoutgv PL_defoutgv
#define PL_Idefstash PL_defstash
#define PL_Idelaymagic PL_delaymagic
+#define PL_Idestroyhook PL_destroyhook
#define PL_Idiehook PL_diehook
#define PL_Idirty PL_dirty
#define PL_Idoextract PL_doextract
--- /var/tmp/perl-current/intrpvar.h 2007-10-19 16:18:19.813159500 -0400
+++ perl-current/intrpvar.h 2007-11-06 10:58:42.893432700 -0500
@@ -663,6 +663,9 @@
PERLVARI(Islab_count, U32, 0) /* Size of the array */
#endif
+/* Can shared object be destroyed */
+PERLVARI(Idestroyhook, destroyable_proc_t, MEMBER_TO_FPTR(Perl_sv_destroyable))
+
/* If you are adding a U8 or U16, check to see if there are 'Space' comments
* above on where there are gaps which currently will be structure padding. */
--- /var/tmp/perl-current/perl.h 2007-11-05 09:54:57.991513600 -0500
+++ perl-current/perl.h 2007-11-06 10:58:27.673319100 -0500
@@ -4036,6 +4036,7 @@
typedef void (CPERLscope(*share_proc_t)) (pTHX_ SV *sv);
typedef int (CPERLscope(*thrhook_proc_t)) (pTHX);
typedef OP* (CPERLscope(*PPADDR_t)[]) (pTHX);
+typedef bool (CPERLscope(*destroyable_proc_t)) (pTHX_ SV *sv);
/* _ (for $_) must be first in the following list (DEFSV requires it) */
#define THREADSV_NAMES "_123456789&`'+/.,\\\";^-%=|~:\001\005!@"
--- /var/tmp/perl-current/sv.c 2007-11-05 09:54:16.390226900 -0500
+++ perl-current/sv.c 2007-11-06 13:09:30.287385900 -0500
@@ -5098,7 +5098,9 @@
}
if (SvOBJECT(sv)) {
- if (PL_defstash) { /* Still have a symbol table? */
+ if (PL_defstash && /* Still have a symbol table? */
+ SvDESTROYABLE(sv))
+ {
dSP;
HV* stash;
do {
@@ -11365,6 +11367,7 @@
PL_lockhook = proto_perl->Ilockhook;
PL_unlockhook = proto_perl->Iunlockhook;
PL_threadhook = proto_perl->Ithreadhook;
+ PL_destroyhook = proto_perl->Idestroyhook;
#ifdef THREADS_HAVE_PIDS
PL_ppid = proto_perl->Ippid;
--- /var/tmp/perl-current/sv.h 2007-10-19 16:18:23.913511900 -0400
+++ perl-current/sv.h 2007-11-05 12:42:48.390289600 -0500
@@ -2008,6 +2008,7 @@
#define SvSHARE(sv) CALL_FPTR(PL_sharehook)(aTHX_ sv)
#define SvLOCK(sv) CALL_FPTR(PL_lockhook)(aTHX_ sv)
#define SvUNLOCK(sv) CALL_FPTR(PL_unlockhook)(aTHX_ sv)
+#define SvDESTROYABLE(sv) CALL_FPTR(PL_destroyhook)(aTHX_ sv)
#define SvGETMAGIC(x) STMT_START { if (SvGMAGICAL(x)) mg_get(x); } STMT_END
#define SvSETMAGIC(x) STMT_START { if (SvSMAGICAL(x)) mg_set(x); } STMT_END
--- /var/tmp/perl-current/util.c 2007-10-25 09:07:11.210225100 -0400
+++ perl-current/util.c 2007-11-06 10:46:33.499959900 -0500
@@ -5112,6 +5112,26 @@
PERL_UNUSED_ARG(sv);
}
+/*
+
+=for apidoc sv_destroyable
+
+Dummy routine which reports that object can be destroyed when there is no
+sharing module present. It ignores its single SV argument, and returns
+'true'. Exists to avoid test for a NULL function pointer and because it
+could potentially warn under some level of strict-ness.
+
+=cut
+*/
+
+bool
+Perl_sv_destroyable(pTHX_ SV *sv)
+{
+ PERL_UNUSED_CONTEXT;
+ PERL_UNUSED_ARG(sv);
+ return TRUE;
+}
+
U32
Perl_parse_unicode_opts(pTHX_ const char **popt)
{
shared.patch
(application/octet-stream, 6.2 KB)
diff -ruN perl-current/MANIFEST perl-patched/MANIFEST
--- perl-current/MANIFEST 2007-11-06 12:56:42.969535800 -0500
+++ perl-patched/MANIFEST 2007-11-06 12:56:48.282613800 -0500
@@ -1116,6 +1116,7 @@
ext/threads/shared/t/hv_refs.t Test shared hashes containing references
ext/threads/shared/t/hv_simple.t Tests for basic shared hash functionality.
ext/threads/shared/t/no_share.t Tests for disabled share on variables.
+ext/threads/shared/t/object.t Tests shared objects in shared structures
ext/threads/shared/t/shared_attr.t Test :shared attribute
ext/threads/shared/t/stress.t Stress test
ext/threads/shared/t/sv_refs.t thread shared variables
diff -ruN perl-current/ext/threads/shared/shared.pm perl-patched/ext/threads/shared/shared.pm
--- perl-current/ext/threads/shared/shared.pm 2007-11-06 12:56:42.875775600 -0500
+++ perl-patched/ext/threads/shared/shared.pm 2007-11-06 12:56:43.688364000 -0500
@@ -5,7 +5,7 @@
use strict;
use warnings;
-our $VERSION = '1.14';
+our $VERSION = '1.15';
my $XS_VERSION = $VERSION;
$VERSION = eval $VERSION;
@@ -73,7 +73,7 @@
=head1 VERSION
-This document describes threads::shared version 1.14
+This document describes threads::shared version 1.15
=head1 SYNOPSIS
@@ -368,7 +368,7 @@
L<http://www.cpanforum.com/dist/threads-shared>
Annotated POD for L<threads::shared>:
-L<http://annocpan.org/~JDHEDDEN/threads-shared-1.14/shared.pm>
+L<http://annocpan.org/~JDHEDDEN/threads-shared-1.15/shared.pm>
Source repository:
L<http://code.google.com/p/threads-shared/>
diff -ruN perl-current/ext/threads/shared/shared.xs perl-patched/ext/threads/shared/shared.xs
--- perl-current/ext/threads/shared/shared.xs 2007-11-06 12:56:42.875775600 -0500
+++ perl-patched/ext/threads/shared/shared.xs 2007-11-06 12:56:43.688364000 -0500
@@ -1108,6 +1108,24 @@
}
+/* Can a shared object be destroyed?
+ * True if not a shared,
+ * or if detroying last proxy on a shared object
+ */
+#ifdef PL_destroyhook
+bool
+Perl_shared_object_destroy(pTHX_ SV *sv)
+{
+ SV *ssv;
+
+ if (SvROK(sv))
+ sv = SvRV(sv);
+ ssv = Perl_sharedsv_find(aTHX_ sv);
+ return (!ssv || (SvREFCNT(ssv) <= 1));
+}
+#endif
+
+
/* Saves a space for keeping SVs wider than an interpreter. */
void
@@ -1121,6 +1139,9 @@
recursive_lock_init(aTHX_ &PL_sharedsv_lock);
PL_lockhook = &Perl_sharedsv_locksv;
PL_sharehook = &Perl_sharedsv_share;
+#ifdef PL_destroyhook
+ PL_destroyhook = &Perl_shared_object_destroy;
+#endif
}
#endif /* USE_ITHREADS */
diff -ruN perl-current/ext/threads/shared/t/object.t perl-patched/ext/threads/shared/t/object.t
--- perl-current/ext/threads/shared/t/object.t 1969-12-31 19:00:00.000000000 -0500
+++ perl-patched/ext/threads/shared/t/object.t 2007-11-06 12:56:43.907137800 -0500
@@ -0,0 +1,151 @@
+use strict;
+use warnings;
+
+BEGIN {
+ if ($ENV{'PERL_CORE'}){
+ chdir 't';
+ unshift @INC, '../lib';
+ }
+ use Config;
+ if (! $Config{'useithreads'}) {
+ print("1..0 # Skip: Perl not compiled with 'useithreads'\n");
+ exit(0);
+ }
+ if ($] < 5.010) {
+ print("1..0 # Skip: Needs Perl 5.10.0 or later\n");
+ exit(0);
+ }
+}
+
+use ExtUtils::testlib;
+
+BEGIN {
+ $| = 1;
+ print("1..23\n"); ### Number of tests that will be run ###
+};
+
+use threads;
+use threads::shared;
+
+my $TEST;
+BEGIN {
+ share($TEST);
+ $TEST = 1;
+}
+
+sub ok {
+ my ($ok, $name) = @_;
+
+ lock($TEST);
+ my $id = $TEST++;
+
+ # You have to do it this way or VMS will get confused.
+ if ($ok) {
+ print("ok $id - $name\n");
+ } else {
+ print("not ok $id - $name\n");
+ printf("# Failed test at line %d\n", (caller)[2]);
+ }
+
+ return ($ok);
+}
+
+ok(1, 'Loaded');
+
+### Start of Testing ###
+
+{ package Jar;
+ my @jar :shared;
+
+ sub new
+ {
+ bless(&threads::shared::share({}), shift);
+ }
+
+ sub store
+ {
+ my ($self, $cookie) = @_;
+ push(@jar, $cookie);
+ return $jar[-1]; # Reaults in destruction of proxy object
+ }
+
+ sub peek
+ {
+ return $jar[-1];
+ }
+
+ sub fetch
+ {
+ pop(@jar);
+ }
+}
+
+{ package Cookie;
+
+ sub new
+ {
+ my $self = bless(&threads::shared::share({}), shift);
+ $self->{'type'} = shift;
+ return $self;
+ }
+
+ sub DESTROY
+ {
+ delete(shift->{'type'});
+ }
+}
+
+my $C1 = 'chocolate chip';
+my $C2 = 'oatmeal raisin';
+my $C3 = 'vanilla wafer';
+
+my $cookie = Cookie->new($C1);
+ok($cookie->{'type'} eq $C1, 'Have cookie');
+
+my $jar = Jar->new();
+$jar->store($cookie);
+
+ok($cookie->{'type'} eq $C1, 'Still have cookie');
+ok($jar->peek()->{'type'} eq $C1, 'Still have cookie');
+ok($cookie->{'type'} eq $C1, 'Still have cookie');
+
+threads->create(sub {
+ ok($cookie->{'type'} eq $C1, 'Have cookie in thread');
+ ok($jar->peek()->{'type'} eq $C1, 'Still have cookie in thread');
+ ok($cookie->{'type'} eq $C1, 'Still have cookie in thread');
+
+ $jar->store(Cookie->new($C2));
+ ok($jar->peek()->{'type'} eq $C2, 'Added cookie in thread');
+})->join();
+
+ok($cookie->{'type'} eq $C1, 'Still have original cookie after thread');
+ok($jar->peek()->{'type'} eq $C2, 'Still have added cookie after thread');
+
+$cookie = $jar->fetch();
+ok($cookie->{'type'} eq $C2, 'Fetched cookie from jar');
+ok($jar->peek()->{'type'} eq $C1, 'Cookie still in jar');
+
+$cookie = $jar->fetch();
+ok($cookie->{'type'} eq $C1, 'Fetched cookie from jar');
+undef($cookie);
+
+share($cookie);
+$cookie = $jar->store(Cookie->new($C3));
+ok($jar->peek()->{'type'} eq $C3, 'New cookie in jar');
+ok($cookie->{'type'} eq $C3, 'Have cookie');
+
+threads->create(sub {
+ ok($cookie->{'type'} eq $C3, 'Have cookie in thread');
+ $cookie = Cookie->new($C1);
+ ok($cookie->{'type'} eq $C1, 'Change cookie in thread');
+ ok($jar->peek()->{'type'} eq $C3, 'Still have cookie in jar');
+})->join();
+
+ok($cookie->{'type'} eq $C1, 'Have changed cooke after thread');
+ok($jar->peek()->{'type'} eq $C3, 'Still have cookie in jar');
+undef($cookie);
+ok($jar->peek()->{'type'} eq $C3, 'Still have cookie in jar');
+$cookie = $jar->fetch();
+ok($cookie->{'type'} eq $C3, 'Fetched cookie from jar');
+
+# EOF