Skip Menu |
 

This queue is for tickets about the Scope-Guard CPAN distribution.

Report information
The Basics
Id: 38506
Status: open
Priority: 0/
Queue: Scope-Guard

People
Owner: Nobody in particular
Requestors: todd.gardner [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 0.03
Fixed in: 0.03



Subject: Scope::Guard isn't threadsafe
Download (untitled) / with headers
text/plain 1.4k
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
Download no_clone.diff
text/x-diff 1k
--- ..\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');
Download (untitled) / with headers
text/plain 179b
Hi, Todd. I'm totally at your mercy on this as I don't play with perl threads. I'll apply the patch you prefer and let you know when it's done. Thank you so much, chocolateboy.
Download (untitled) / with headers
text/plain 950b
On 2008-08-15 15:29:52, CHOCOLATE wrote: Show quoted text
> Hi, Todd. > > I'm totally at your mercy on this as I don't play with perl threads. > > I'll apply the patch you prefer and let you know when it's done. >
It does not look like the patch is applied yet. Nevertheless, the problem also occurs in unthreaded perls when fork() is used. A sample script: #!/usr/bin/perl use Scope::Guard qw(guard); my $guard = guard { warn "Called in $$" }; if (fork == 0) { # #$guard->dismiss; # use POSIX; POSIX::_exit(0); exit 0; } __END__ The callback will be called twice. The possible workarounds are in comments: either call dismiss or use POSIX::_exit in the child. But it would be nice if Scope::Guard would remember in which process the guard was created, and call the callback only in that process. This would be similar like the core module File::Temp is doing this. See the "Forking" section in the File::Temp documentation. Best regards, Slaven
Download (untitled) / with headers
text/plain 1.1k
Hi, Slaven. Thanks for commenting. I've used fork() in two Perl projects in the past 15 years, and have never used Perl threads, hence my epic slowness/hesitation (i.e. laziness :-) on this. Are you sure the fork behaviour you describe is what's always wanted? If I'm using Scope::Guard to restore the current working directory at the end of a block, then I'd be quite happy for both processes to fire the guard handler - and by "happy", I mean I'd *require* it. Admittedly, if both processes were cleaning up a file, then the duplication might be unwelcome, but that, as you point out, could easily be handled manually with an explicit call to dismiss() (or inside the handler). Either way, I've just uploaded Scope::Guard to GitHub for easy hacking, and I'm happy to add you and/or Todd as co-maintainers on PAUSE if this is a showstopper. http://github.com/chocolateboy/Scope-Guard I'm fine with any changes as long as a) nothing breaks on single-threaded/process perls (with tests to confirm this), and b) the utility of the fork() behaviour you've outlined above outweighs its (potential) inconvenience. Thanks, chocolateboy.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.