Your problem is open(file, ">$mailclicks"); It will clobber the file before it gets the lock.
Also, you are doing a lot of open and flocks without checking their results - which means you'll keep running with now faulty data that will subsequently be used to touch files it should never reach.
Not to mention you have a race condition because you slurp the file but then unlock and close it. Then you reopen it and write back your results. In the meantime, another process may update the data, causing your updates to become outdated.
Also, revoking the lock before closing is a bad idea (I can't quite remember where it is, but merlyn had an excellent writeup on it) - closing the file will revoke the lock anyway so just leave well enough alone.
use Fcntl qw(:DEFAULT :flock :seek);
sysopen FILE, $mailclicks, O_RDWR | O_CREAT or die "couldn't open $mai
+lclicks: $!";
flock FILE, LOCK_EX or die "failed acquiring lock on $mailclicks: $!";
my @slurp = <FILE>;
# do something with @slurp
seek FILE, 0, SEEK_SET; # go back to top of file
print FILE @slurp;
truncate FILE, tell FILE; # because the file may be smaller than it wa
+s before
close FILE;
Note you should use chomp rather than s/\n//.
But what is your loop doing there? I see a lot of very roundabout ways for doing things. First of all, you iterate over @rec. But you want to modify the current element in the if block, so you keep track of the current element in $i. That is entirely unnecessary: the loop variable, $rec in your case, is an alias to the current element. If you modify it, the actual array element will be modified. In your case you can skip the $i business and simply write $rec = rather than $recs[$i] =.
Another thing I don't understand is why you glue the splitted values back together. The string you create to assign to $recs[$i] is exactly what you had at the beginning of the loop. You could just write $rec and get the same result.
Now the funny part is we have simplified that assignment to $rec = $rec.
All that happens within the if is that $received gets updated. And since you did not modifiy the initial data, all that happens when you write the file back is that empty lines are removed. For that result, you're doing far too much work.
Since all you use is the $id part, you you can also leave out the chomp entirely and limit your split to return only the part up to the first ::.
Your entire code could be simplified to the following:
use Fcntl qw(:DEFAULT :flock :seek);
sysopen FILE, $mailclicks, O_RDWR | O_CREAT or die "couldn't open $mai
+lclicks: $!";
flock FILE, LOCK_EX or die "failed acquiring lock on $mailclicks: $!";
my @records = <FILE>;
seek FILE, 0, SEEK_SET;
for(@records) { # now the line will be in $_ rather than $rec
next if $_ eq "\n"; # skip empty lines completely
my ($id) = split /::/, $_, 2;
++$received if $id eq $formdata{id};
print FILE $_;
}
truncate FILE, tell FILE;
close FILE;
Much clearer, and no race conditions.
From the example you post it also appears as though you're using neither strict nor warnings. That's a bad idea; you're inviting subtle and hard to find bugs caused by typos that Perl would easily spot for you.
Makeshifts last the longest. | 