Your code still exhibits a race condition between the point of generation of the 'unique' name and the later use of it in the code - There is no subsequent check for existence of uniqueness at the point of usage.
While your code certainly has merit, it would be better written within a while loop, actually creating the destination object rather than simply determing the object name, in the event of the available 'unique' name. Alternatively, there still exists the possibility that a similarly named object is created between the time of your determination of the available unique name and the point of object creation. I dicussed this point in a recent meditation I posted with regard to the usage of temporary files within Perl here.
perl -e 's&&rob@cowsnet.com.au&&&split/[@.]/&&s&.com.&_&&&print' | [reply] |
Thanks for the comment, rob_au.
Suppose it were used as follows; I can imagine
that neck and neck races would make it go very slow, but would it fail?
#!/usr/bin/perl -w
use strict;
my $files_like = 'aaa';
for (1..100){
my $next_file = nextuniq($files_like,[map {m/tmp_(\D+)\.txt$/} glob
+('./tmp_*.txt')]);
while (-e "./tmp_$next_file.txt"){
$next_file = nextuniq($next_file,[map {m/tmp_(\D+)\.txt$/} glob(
+'./tmp_*.txt')]);
print "race condition\n";
}
open TEMP,">./tmp_$next_file.txt" or die;
# do something
close TEMP;
}
sub nextuniq{
my ($test_item,$item_list) = @_;
my $new_item;
my $found = 1;
if (! UNIVERSAL::isa( $item_list, "ARRAY" )){
require Carp;
Carp::croak( "Arg 2 must be an array reference\n");
}
while ($found) {
$found = 0;
foreach $new_item (@$item_list) {
if ($new_item eq $test_item) {
$test_item++;
$found = 1;
}
}
}
$new_item = $test_item;
$new_item;
}
If it would fail, can it be written so that it can't fail?
mkmcconn
| [reply]
[d/l] |
Now, first of all, from this point on, I can quite willing to be corrected, however to my mind in order to prevent a race condition, the very next operation which should be carried out following the determination of the file name is the acquisition of an exclusive lock on the file. While your code iterates through the loop until a non-existent file name is determined, to my thinking, the next operation must be to open the file and acquire the lock rather than the test for existent (which should fail, thereby exiting the loop and allowing the file to subsequently opened) - The difference in code is minute and frankly I don't really know if its absolutely necessary, however, if I were requested to rewrite the code, the following is how I would rewrite it:
(Note that this philosophy with regard to order of actions to prevent race conditions with temporary files is based upon discussions within the comp.lang.perl.moderated newsgroup thread here and a BUGTRAQ post from Tom Christiansen here)
#!/usr/bin/perl
use Fcntl;
use strict;
my $fname = nextunique( 'aaa', [ map { m/tmp_(\D+).txt$/ } glob('./tmp
+_*.txt') ] );
do {
$fname = nextunique( $fname, [ map { m/tmp_(\D+).txt$/ } glob('./t
+mp_*.txt') ] );
} until open (FH, $fname, O_RDWR|O_CREAT|O_EXCL, 0666);
# code follows
close FH;
sub nextunique {
my ($test_item, $item_list) = @_;
exit 1 unless UNIVERSAL::isa( $item_list, 'ARRAY' );
foreach ( @{ $item_list } ) {
if ( $_ eq $test_item ) {
++$test_item;
last;
}
}
return $test_item;
}
You will also note that I have removed the while loop within the nextunique subroutine, thereby minimising the chance of an infinite loop should the element $new_item not exist within @{ $item_list }. A more defensive approach to programming? Maybe, maybe not ...
perl -e 's&&rob@cowsnet.com.au&&&split/[@.]/&&s&.com.&_&&&print'
| [reply]
[d/l]
[select] |
