| Subject: | Scope::Guard isn't threadsafe |
Hi,
Working with scope guard, I've found doing the following:
use threads;
use Scope::Guard;
my $sg = Scope::Guard->new( sub { print "Scope Guard Fired\n"; } );
my $t = async { };
$t->join;
Will cause the guarded function to be called twice. Depending on the
guarded function, this could be fairly bad, and since this would happen
any time a thread is created while a scope guard is active, it could
occur function calls below where the scope guard is created, without the
author of the original code scope guarded realizing.
I've attached two diffs of solutions, the first (no_clone.diff) which
just adds "sub CLONE_SKIP { 1 }", effectively making the scope guard not
be transferred to the child classes, and the second version
(thread_dismissable.diff), which I prefer, where the scope guard, if
lexically accessible, can be dismissed from child threads, but not
delayed, so, you can write:
{
my $sg = Scope::Guard->new(sub { ++$i });
my $t = async(sub { $sg->dismiss(1); });
$t->join();
}
In the no_clone version, this will produce an error because $sg is
unblessed, while in the thread_dismissable, this will properly dismiss
the scope guard so it won't be fired in its native thread. I've updated
the POD to reflect that in thread_dismissable version.
(This is my first submission on CPAN, and I couldn't find guidelines for
the diff format. I hope unified is okay, but if not, I can submit it
whatever format you prefer)
Thank you for your time,
Todd Gardner
| Subject: | thread_dismissable.diff |
--- ..\Scope-Guard-0.03-1CVsDJ\/lib/Scope/Guard.pm 2007-01-07 13:17:47.000000000 -0500
+++ ./lib/Scope/Guard.pm 2008-08-15 11:43:06.514168300 -0400
@@ -2,11 +2,15 @@
use strict;
use warnings;
+use threads::shared qw( share );
+use Scalar::Util qw( refaddr weaken );
use vars qw($VERSION);
$VERSION = '0.03';
+my %REGISTRY;
+
sub new {
my $class = shift;
my $handler = shift() || die "Scope::Guard::new: no handler supplied";
@@ -15,21 +19,43 @@
die "Scope::Guard::new: invalid handler - expected CODE ref, got: '$ref'"
unless (UNIVERSAL::isa($handler, 'CODE'));
- bless [ 0, $handler ], ref $class || $class;
+ my $dismiss = 0;
+ my $self = bless [ share($dismiss), $handler, 1 ], ref $class || $class;
+
+ my $id = refaddr $self;
+ weaken ( $REGISTRY{ $id } = $self );
+
+ return $self;
}
sub dismiss {
my $self = shift;
my $dismiss = @_ ? shift : 1;
- $self->[0] = $dismiss;
+
+ lock $self->[0];
+ ${$self->[0]} = $dismiss;
+}
+
+sub CLONE {
+ foreach my $guard ( values %REGISTRY ) {
+ $guard->[2] = 0;
+ }
+
+ %REGISTRY = ();
}
sub DESTROY {
my $self = shift;
- my ($dismiss, $handler) = @$self;
+ my ($dismiss, $handler, $native_thread) = @$self;
+
+ return unless $native_thread;
+
+ my $id = refaddr $self;
+ delete $REGISTRY{ $id };
- $handler->() unless ($dismiss);
+ lock $dismiss;
+ $handler->() unless (${$dismiss});
}
1;
--- ..\Scope-Guard-0.03-1CVsDJ\/t/Scope-Guard.t 2007-01-07 13:17:47.000000000 -0500
+++ ./t/Scope-Guard.t 2008-08-15 12:09:23.243009300 -0400
@@ -5,11 +5,12 @@
use strict;
use warnings;
-use Test::More tests => 8;
+use Test::More tests => 13;
-BEGIN { use_ok('Scope::Guard') };
+BEGIN { use_ok('threads'); use_ok('threads::shared'); use_ok('Scope::Guard') };
-my $i = 1;
+my $i :shared;
+$i = 1;
{
my $sg = Scope::Guard->new(sub { ok($i++ == 1, 'handler invoked at scope end') });
@@ -43,7 +44,24 @@
ok($i++ == 5, 'dismiss(1) disables handler');
{
- my $sg = Scope::Guard->new(sub { ok($i++ == 6, 'dismiss(0) enables handler') });
+ my $sg = Scope::Guard->new(sub { ok($i == 6, 'dismiss(0) enables handler') });
$sg->dismiss();
$sg->dismiss(0);
}
+
+{
+ my $sg = Scope::Guard->new(sub { ++$i });
+ my $t = async(sub { });
+ $t->join();
+}
+
+ok($i++ == 7, 'Scope::Guard should only fire once if threads are launched');
+
+{
+ my $sg = Scope::Guard->new(sub { ++$i });
+ my $t = async(sub { $sg->dismiss(1); });
+ $t->join();
+}
+
+ok($i++ == 8, 'dismiss() from thread disables handler');
+
| Subject: | no_clone.diff |
--- ..\Scope-Guard-0.03-1CVsDJ\/lib/Scope/Guard.pm 2007-01-07 13:17:47.000000000 -0500
+++ ./lib/Scope/Guard.pm 2008-08-15 11:26:20.992108000 -0400
@@ -25,6 +25,8 @@
$self->[0] = $dismiss;
}
+sub CLONE_SKIP { 1 }
+
sub DESTROY {
my $self = shift;
my ($dismiss, $handler) = @$self;
--- ..\Scope-Guard-0.03-1CVsDJ\/t/Scope-Guard.t 2007-01-07 13:17:47.000000000 -0500
+++ ./t/Scope-Guard.t 2008-08-15 11:26:02.304488400 -0400
@@ -5,11 +5,12 @@
use strict;
use warnings;
-use Test::More tests => 8;
+use Test::More tests => 11;
-BEGIN { use_ok('Scope::Guard') };
+BEGIN { use_ok('threads'); use_ok('threads::shared'); use_ok('Scope::Guard') };
-my $i = 1;
+my $i :shared;
+$i = 1;
{
my $sg = Scope::Guard->new(sub { ok($i++ == 1, 'handler invoked at scope end') });
@@ -47,3 +48,11 @@
$sg->dismiss();
$sg->dismiss(0);
}
+
+{
+ my $sg = Scope::Guard->new(sub { ++$i });
+ my $t = async(sub { });
+ $t->join();
+}
+
+ok($i == 8, 'Scope::Guard should only fire once if threads are launched');