Bug #103054 for FindBin-libs: Race conditions cause failure under parallel-testing

Tue Mar 24 02:35:15 2015 KENTNL [...] cpan.org - Ticket created

Subject:

Race conditions cause failure under parallel-testing

PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/00-cwd-sanity.t ............. ok
t/01-use-ok.t ................. ok
t/02-export-base.t ............ ok
t/04-export-nouse-base.t ...... ok
t/05-export-subdir.t .......... ok
t/06-base-subdir-subonly.t .... ok
t/08-base-subdir-subonly.t .... ok
Show quoted text

===( 17;0 1/1 0/? )==============================================

Show quoted text

# Failed test 'Found only foo subdir'
# at t/07-export-subdir-subonly.t line 15.
# Looks like you failed 1 test of 1.

t/07-export-subdir-subonly.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
t/09-base-subdir-scalar.t ..... ok

Test Summary Report
-------------------
t/07-export-subdir-subonly.t (Wstat: 256 Tests: 1 Failed: 1)
Failed test: 1
Non-zero exit status: 1

Tue Mar 24 03:59:57 2015 KENTNL [...] cpan.org - Correspondence added

I couldn't find a source controlled version, so here is a patch vs last CPAN release that patches out the risks.

It does this by making sure any test that creates or deletes elements do so with unique tokens that are guaranteed to be unique even if somebody later blindly cargocults a test., and it does this by storing both the test name ($0) and the pid ($$) ( with some mutations )in the subject files.

My only real concerns with this approach is the potential to create filenames larger than a system wants to accomodate, but that is in practice a rare problem.

Obviously, for other readers I may forward this bug to for code review, using File::Temp is very much not an option here without a lot of convoluted tricks and even more fragile logic, due to the nature of FindBin needing $0 to be in a specific location relative to CWD in order for it to work, thus, as soon as one CHDIRs out from the PROJECTROOT, all the test logic begins to fail.

Subject:

patch.txt

diff -Naur a/FindBin-libs-2.12/t/02-export-base.t b/FindBin-libs-2.12/t/02-export-base.t --- a/FindBin-libs-2.12/t/02-export-base.t 2014-07-28 07:38:56.000000000 +0000 +++ b/FindBin-libs-2.12/t/02-export-base.t 2015-03-24 07:37:09.994863874 +0000 @@ -19,20 +19,23 @@ # # eval necessary for crippled O/S w/ missing/broken symlinks. +my $suffix; BEGIN { - eval { symlink qw( /nonexistant/path/to/foobar ./foobar ) } + $suffix="$$.$0"; + $suffix=~s/\W/_/g; + eval { symlink '/nonexistant/path/to/foobar', './foobar-' . $suffix } } END { - unlink './foobar'; + unlink './foobar-' . $suffix; } use FindBin::libs qw( export ); use FindBin::libs qw( export=found base=blib ); use FindBin::libs qw( export=junk base=frobnicatorium ); -use FindBin::libs qw( export base=foobar ); +use FindBin::libs 'export', 'base=foobar-' . $suffix; my %testz = qw @@ -40,13 +43,14 @@ lib 1 found 1 junk 0 - foobar 0 ); +$testz{ 'foobar-' . $suffix } = 0; plan tests => 1 * keys %testz; -while( my ($name, $true) = each %testz ) +for my $name ( sort keys %testz ) { + my $true = $testz{$name}; my $dest = qualify $name; my $ref = qualify_to_ref $dest; diff -Naur a/FindBin-libs-2.12/t/05-export-subdir.t b/FindBin-libs-2.12/t/05-export-subdir.t --- a/FindBin-libs-2.12/t/05-export-subdir.t 2014-07-27 14:29:58.000000000 +0000 +++ b/FindBin-libs-2.12/t/05-export-subdir.t 2015-03-24 07:41:13.738445608 +0000 @@ -7,13 +7,18 @@ $\ = "\n"; $, = "\n\t"; -BEGIN { -d './lib/foo' || mkdir './lib/foo', 0555 or die $! } -END { -d './lib/foo' && rmdir './lib/foo' or die $! } +my $suffix; +BEGIN { + $suffix = "$$.$0"; + $suffix =~ s/\W/_/g; +} +BEGIN { -d './lib/foo-' . $suffix || mkdir './lib/foo-' . $suffix, 0555 or die $! } +END { -d './lib/foo-' . $suffix && rmdir './lib/foo-' . $suffix or die $! } -use FindBin::libs qw( export subdir=foo ); +use FindBin::libs 'export', 'subdir=foo-' . $suffix; -my $found = grep m{\bfoo\b}, @lib; +my $found = grep m{\bfoo-\Q$suffix\E\b}, @lib; -ok $found, 'Found foo subdir'; +ok $found, "Found foo-$suffix subdir"; __END__ diff -Naur a/FindBin-libs-2.12/t/07-export-subdir-subonly.t b/FindBin-libs-2.12/t/07-export-subdir-subonly.t --- a/FindBin-libs-2.12/t/07-export-subdir-subonly.t 2014-07-26 08:14:19.000000000 +0000 +++ b/FindBin-libs-2.12/t/07-export-subdir-subonly.t 2015-03-24 07:43:27.627145219 +0000 @@ -4,14 +4,18 @@ $\ = "\n"; $, = "\n\t"; +my $suffix; +BEGIN { + $suffix = "$$.$0"; + $suffix =~ s/\W/_/g; +} +BEGIN { mkdir './lib/foo-' . $suffix, 0555 } +END { rmdir './lib/foo-' . $suffix } -BEGIN { mkdir './lib/foo', 0555 } -END { rmdir './lib/foo' } - -use FindBin::libs qw( export subdir=foo subonly ); +use FindBin::libs 'export', 'subdir=foo-' . $suffix, 'subonly'; use Test::More tests => 1; -ok @lib == 1, 'Found only foo subdir'; +ok @lib == 1, "Found only foo-$suffix subdir"; __END__ diff -Naur a/FindBin-libs-2.12/t/08-base-subdir-subonly.t b/FindBin-libs-2.12/t/08-base-subdir-subonly.t --- a/FindBin-libs-2.12/t/08-base-subdir-subonly.t 2014-07-27 14:29:58.000000000 +0000 +++ b/FindBin-libs-2.12/t/08-base-subdir-subonly.t 2015-03-24 07:46:10.384695372 +0000 @@ -6,16 +6,21 @@ use Test::More tests => 2; -BEGIN { mkdir './blib/foo', 0555 } -END { rmdir './blib/foo' } +my $suffix; +BEGIN { + $suffix="$$.$0"; + $suffix=~s/\W/_/g; +} +BEGIN { mkdir './blib/foo-'.$suffix, 0555 } +END { rmdir './blib/foo-'.$suffix } require FindBin::libs; -FindBin::libs->import( qw( base=blib subdir=foo subonly ) ); +FindBin::libs->import( 'base=blib', 'subdir=foo-' . $suffix, 'subonly' ); -my $expect = catpath '' => qw( blib foo ); +my $expect = catpath '' => ('blib','foo-' . $suffix ); -like $INC[0], qr{\Q$expect\E $}x, 'Found only foo subdir'; +like $INC[0], qr{\Q$expect\E $}x, "Found only foo-$suffix subdir"; FindBin::libs->import; diff -Naur a/FindBin-libs-2.12/t/09-base-subdir-scalar.t b/FindBin-libs-2.12/t/09-base-subdir-scalar.t --- a/FindBin-libs-2.12/t/09-base-subdir-scalar.t 2015-01-15 17:39:03.000000000 +0000 +++ b/FindBin-libs-2.12/t/09-base-subdir-scalar.t 2015-03-24 07:48:34.183298720 +0000 @@ -7,8 +7,13 @@ use Test::More tests => 2; -BEGIN { mkdir './blib/blort', 0555 } -END { rmdir './blib/blort' } +my $suffix; +BEGIN { + $suffix="$$.$0"; + $suffix=~s/\W/_/g; +} +BEGIN { mkdir './blib/blort-' . $suffix, 0555 } +END { rmdir './blib/blort-' . $suffix } require FindBin::libs; @@ -17,25 +22,21 @@ 2.0 < FindBin::libs->VERSION or skip "Test for new version", 2; - FindBin::libs->import - ( - qw - ( - base=blib - subdir=blort - subonly - export=snark - scalar - ) + FindBin::libs->import( + 'base=blib', + 'subdir=blort-' . $suffix, + 'subonly', + 'export=snark', + 'scalar', ); my $ref = qualify_to_ref 'snark'; - my $expect = catpath '' => qw( blib blort ); + my $expect = catpath '' => 'blib', 'blort-' . $suffix; my $value = ${ *$ref }; ok $value, "Exported scalar '\$snark'"; - like $value, qr{\Q$expect\E $}x, "Found 'blib/blort' ($value)"; + like $value, qr{\Q$expect\E $}x, "Found 'blib/blort-$suffix' ($value)"; }

Tue Mar 24 04:00:04 2015 KENTNL [...] cpan.org - Fixed in 2.12 added

Tue Mar 24 04:00:23 2015 KENTNL [...] cpan.org - Fixed in 2.12 deleted

Tue Mar 24 06:10:39 2015 KENTNL [...] cpan.org - Correspondence added

I've made a second attempt at the patch, which in fact uses File::Temp without the craziness I was afraid of.

And the added benefit is this notation avoids the need to manually handle the cleanup logic.

Subject:

patch_2.txt

diff -Naur META.json META.json --- META.json 2015-01-16 06:43:04.000000000 +1300 +++ META.json 2015-03-24 23:02:56.004805137 +1300 @@ -4,7 +4,7 @@ "unknown" ], "dynamic_config" : 1, - "generated_by" : "ExtUtils::MakeMaker version 7.04, CPAN::Meta::Converter version 2.143240", + "generated_by" : "ExtUtils::MakeMaker version 7.04, CPAN::Meta::Converter version 2.150001", "license" : [ "unknown" ], @@ -34,6 +34,7 @@ "requires" : { "Carp" : "0", "Cwd" : "0", + "File::Basename" : "0", "File::Spec" : "0", "File::Temp" : "0", "FindBin" : "0", diff -Naur META.yml META.yml --- META.yml 2015-01-16 06:43:04.000000000 +1300 +++ META.yml 2015-03-24 23:02:53.529810608 +1300 @@ -7,7 +7,7 @@ configure_requires: ExtUtils::MakeMaker: '0' dynamic_config: 1 -generated_by: 'ExtUtils::MakeMaker version 7.04, CPAN::Meta::Converter version 2.143240' +generated_by: 'ExtUtils::MakeMaker version 7.04, CPAN::Meta::Converter version 2.150001' license: unknown meta-spec: url: http://module-build.sourceforge.net/META-spec-v1.4.html @@ -20,6 +20,7 @@ requires: Carp: '0' Cwd: '0' + File::Basename: '0' File::Spec: '0' File::Temp: '0' FindBin: '0' diff -Naur Makefile.PL Makefile.PL --- Makefile.PL 2015-01-16 06:42:22.000000000 +1300 +++ Makefile.PL 2015-03-24 23:01:45.809960001 +1300 @@ -44,6 +44,7 @@ Cwd 0 FindBin 0 Symbol 0 + File::Basename 0 File::Spec 0 File::Temp 0 List::Util 0 diff -Naur t/02-export-base.t t/02-export-base.t --- t/02-export-base.t 2014-07-28 19:38:56.000000000 +1200 +++ t/02-export-base.t 2015-03-24 22:45:51.952000767 +1300 @@ -8,6 +8,8 @@ use Symbol; use Test::More; +use File::Temp qw( tempfile ); +use File::Basename qw( basename ); $\ = "\n"; $, = "\n\t"; @@ -19,20 +21,21 @@ # # eval necessary for crippled O/S w/ missing/broken symlinks. -BEGIN -{ - eval { symlink qw( /nonexistant/path/to/foobar ./foobar ) } -} +my ( $fh, $file, $basename ); -END -{ - unlink './foobar'; +BEGIN { + ( $fh, $file ) = tempfile( '02-export-XXXXXX', DIR => './', OPEN => 0 ); + $basename = basename($file); + eval { symlink '/nonexistant/path/to/foobar', './' . $basename }; } +END { + unlink $file; # Autocleanup doesn't work here. +} use FindBin::libs qw( export ); use FindBin::libs qw( export=found base=blib ); use FindBin::libs qw( export=junk base=frobnicatorium ); -use FindBin::libs qw( export base=foobar ); +use FindBin::libs 'export', 'base=' . $basename; my %testz = qw @@ -40,13 +43,14 @@ lib 1 found 1 junk 0 - foobar 0 ); +$testz{$basename} = 0; plan tests => 1 * keys %testz; -while( my ($name, $true) = each %testz ) +for my $name ( sort keys %testz ) { + my $true = $testz{$name}; my $dest = qualify $name; my $ref = qualify_to_ref $dest; diff -Naur t/05-export-subdir.t t/05-export-subdir.t --- t/05-export-subdir.t 2014-07-28 02:29:58.000000000 +1200 +++ t/05-export-subdir.t 2015-03-24 22:51:15.953321024 +1300 @@ -3,17 +3,21 @@ use v5.8; use Test::More tests => 1; - +use File::Temp qw( tempdir ); +use File::Basename qw( basename ); $\ = "\n"; $, = "\n\t"; -BEGIN { -d './lib/foo' || mkdir './lib/foo', 0555 or die $! } -END { -d './lib/foo' && rmdir './lib/foo' or die $! } +my ($dir,$basename); +BEGIN { + $dir = tempdir( '05-export-subdir-XXXXXX', DIR => './lib/', CLEANUP => 1 ); + $basename = basename( $dir ); +} -use FindBin::libs qw( export subdir=foo ); +use FindBin::libs 'export', 'subdir=' . $basename; -my $found = grep m{\bfoo\b}, @lib; +my $found = grep m{\b\Q$basename\E\b}, @lib; -ok $found, 'Found foo subdir'; +ok $found, "Found $basename subdir"; __END__ diff -Naur t/07-export-subdir-subonly.t t/07-export-subdir-subonly.t --- t/07-export-subdir-subonly.t 2014-07-26 20:14:19.000000000 +1200 +++ t/07-export-subdir-subonly.t 2015-03-24 22:53:38.147018466 +1300 @@ -1,17 +1,18 @@ package Testophile; use v5.8; +use File::Temp qw( tempdir ); +use File::Basename qw( basename ); +my ($dir,$basename); +BEGIN { + $dir = tempdir( '07-export-subdir-XXXXXX', DIR => './lib/', CLEANUP => 1 ); + $basename = basename( $dir ); +} -$\ = "\n"; -$, = "\n\t"; - -BEGIN { mkdir './lib/foo', 0555 } -END { rmdir './lib/foo' } - -use FindBin::libs qw( export subdir=foo subonly ); +use FindBin::libs 'export', 'subdir=' . $basename, 'subonly'; use Test::More tests => 1; -ok @lib == 1, 'Found only foo subdir'; +ok @lib == 1, "Found only $basename subdir"; __END__ diff -Naur t/08-base-subdir-subonly.t t/08-base-subdir-subonly.t --- t/08-base-subdir-subonly.t 2014-07-28 02:29:58.000000000 +1200 +++ t/08-base-subdir-subonly.t 2015-03-24 22:56:44.697617268 +1300 @@ -5,17 +5,22 @@ use File::Spec::Functions qw( catpath ); use Test::More tests => 2; +use File::Temp qw( tempdir ); +use File::Basename qw( basename ); -BEGIN { mkdir './blib/foo', 0555 } -END { rmdir './blib/foo' } +my ( $dir, $basename ); +BEGIN { + $dir = tempdir( '08-base-subdir-subonly-XXXXXX', DIR => './blib/', CLEANUP => 1 ); + $basename = basename($dir); +} require FindBin::libs; -FindBin::libs->import( qw( base=blib subdir=foo subonly ) ); +FindBin::libs->import( 'base=blib', 'subdir=' . $basename, 'subonly' ); -my $expect = catpath '' => qw( blib foo ); +my $expect = catpath '' => ('blib',$basename ); -like $INC[0], qr{\Q$expect\E $}x, 'Found only foo subdir'; +like $INC[0], qr{\Q$expect\E $}x, "Found only $basename subdir"; FindBin::libs->import; diff -Naur t/09-base-subdir-scalar.t t/09-base-subdir-scalar.t --- t/09-base-subdir-scalar.t 2015-01-16 06:39:03.000000000 +1300 +++ t/09-base-subdir-scalar.t 2015-03-24 23:00:11.357167420 +1300 @@ -4,11 +4,17 @@ use File::Spec::Functions qw( catpath ); use Symbol qw( qualify_to_ref ); +use File::Temp qw( tempdir ); +use File::Basename qw( basename ); use Test::More tests => 2; -BEGIN { mkdir './blib/blort', 0555 } -END { rmdir './blib/blort' } +my ($dir,$basename); + +BEGIN { + $dir = tempdir('09-base-subdir-scalar-XXXXXX', DIR => './blib/', CLEANUP => 1 ); + $basename = basename($dir); +} require FindBin::libs; @@ -17,25 +23,21 @@ 2.0 < FindBin::libs->VERSION or skip "Test for new version", 2; - FindBin::libs->import - ( - qw - ( - base=blib - subdir=blort - subonly - export=snark - scalar - ) + FindBin::libs->import( + 'base=blib', + 'subdir=' . $basename, + 'subonly', + 'export=snark', + 'scalar', ); my $ref = qualify_to_ref 'snark'; - my $expect = catpath '' => qw( blib blort ); + my $expect = catpath '' => 'blib', $basename; my $value = ${ *$ref }; ok $value, "Exported scalar '\$snark'"; - like $value, qr{\Q$expect\E $}x, "Found 'blib/blort' ($value)"; + like $value, qr{\Q$expect\E $}x, "Found 'blib/$basename' ($value)"; }

Sun Jun 07 14:58:36 2015 LEMBARK [...] cpan.org - Correspondence added

On Tue Mar 24 06:10:39 2015, KENTNL wrote: Show quoted text

> I've made a second attempt at the patch, which in fact uses File::Temp > without > the craziness I was afraid of.

Q: Where did you acquire a Makfile.PL to patch? The existing module code uses Build.PL and there is no Makefile.PL in the CPAN distribution. I'm also not sure why I'd want to patch all of the *META* files rather than just the tests themselves? Your updates also used K&R braces; given that this is not for publication nor edited on a VT100 the existing code uses Berkeley braces throughout, which makes applying the patches as-is difficult because I have to re-format the updated code. From what I can see the only issue for parallel testing was replacing static paths with tmpfile entries based on the current pid and program (i.e.., "$$.$0"). If this is correct warn me, I can work in grafting that into the code.

Sun Jun 07 14:58:36 2015 The RT System itself - Status changed from 'new' to 'open'

Sun Jun 07 14:58:37 2015 LEMBARK [...] cpan.org - Status changed from 'open' to 'new'

Sun Jun 07 14:58:37 2015 LEMBARK [...] cpan.org - Taken

Sun Jun 07 15:02:04 2015 KENTNL [...] cpan.org - Correspondence added

On 2015-06-08 06:58:36, LEMBARK wrote: Show quoted text

> > Q: Where did you acquire a Makfile.PL to patch? > > The existing module code uses Build.PL and there is no Makefile.PL in > the CPAN distribution. >

That doesn't seem to be the case. https://metacpan.org/source/LEMBARK/FindBin-libs-2.12 https://metacpan.org/source/LEMBARK/FindBin-libs-2.12/Makefile.PL

Sun Jun 07 15:06:34 2015 KENTNL [...] cpan.org - Correspondence added

On 2015-06-08 06:58:36, LEMBARK wrote: Show quoted text

> > I'm also not sure why I'd want to patch all of the *META* files rather > than just the tests themselves?

The updated metafiles are a result of patching Makefile.PL to declare more dependencies. The tests the way I modified them use File::Basename, which they previously did not. The delta cruft is probably needless, and a side effect of patching a CPAN source instead of a git source tree. Show quoted text

> > Your updates also used K&R braces; given that this is not for > publication nor edited on a VT100 the existing code uses Berkeley > braces throughout, which makes applying the patches as-is difficult > because I have to re-format the updated code. > > From what I can see the only issue for parallel testing was replacing > static paths with tmpfile entries based on the current pid and program > (i.e.., "$$.$0"). > > If this is correct warn me, I can work in grafting that into the code.

My initial attempt used $$ and $0, but I decided the code to do that was far far too messy and confusing, and it was better to use File::Temp mechanics to do the same thing.

Sun Jun 07 15:07:39 2015 LEMBARK [...] cpan.org - Correspondence added

I'm an idiot... used an out-of-date copy of the library. Doing a git pull prior to running the patch would've helped. Trying to re-format the code now...

Sun Jun 07 15:07:39 2015 The RT System itself - Status changed from 'new' to 'open'