Re: replace autom4te output file atomically

[Ben Pfaff]

Ralf Wildenhues <address@hidden> writes:
> * Ben Pfaff wrote on Tue, Aug 12, 2008 at 07:14:09AM CEST:
>
>> diff --git a/bin/autom4te.in b/bin/autom4te.in
>> old mode 100644
>> new mode 100755
>
> Why this mode change?

Oops!  My Emacs is configured to chmod +x scripts when it saves
them, so this is just a mistake.

>> index 685df41..1ea86da
>> --- a/bin/autom4te.in
>> +++ b/bin/autom4te.in
>
>> @@ -550,21 +550,36 @@ sub handle_output ($$)
>
>>    else
>>      {
>> -      $out->open($output, O_CREAT | O_WRONLY | O_TRUNC, oct ($mode));
>> +      $out->open ("$output.tmp", O_CREAT | O_WRONLY | O_TRUNC, oct ($mode));
>
> Hmm.  This may still leave us competing with a concurrent autoconf
> running.  I'm not saying that this is something we need (there are
> more places that need fixing in order to guard against this), but
> shouldn't a temporary file have a temp name (or at least using 
> a PID or so)?
>
> We use mktmpdir earlier, but possibly on a different mount, which
> will break the atomicity if we create the file there.

Good point.  I changed the file name to "$output.tmp$$".

However, now, terminating with a signal leaves a garbage file
that will not be overwritten on the next execution, so to be
polite we must catch fatal signals and delete our temporary file.
So I added code to do that, too.  I'd understand if you want the
code factored out into a new function or a new module, along the
lines of lib/fatal-signal.[ch] and lib/clean-temp.[ch] from
gnulib (especially if autom4te has other temporary files that
want cleaning up in the same way).  Let me know if that's
desirable.

>> @@ -600,6 +615,12 @@ sub handle_output ($$)
>>  
>>    $out->close();
>>  
>> +  if ($atomic_replace && !rename ("$output.tmp", "$output"))
>> +    {
>> +      move ("${output}.tmp", "$output")
>
> Please use a variable for "${output}.tmp".

Done.

>> +    or fatal "cannot rename ${output}.tmp as $output: $!";
>
> How about still trying to remove the temp file?

Fixed.

> A test case to expose the issue fixed by this patch would be nice
> (but I'm not sure how to do it easily).

I added a simple test case, although I am not sure about the
portability of $PPID.  Also, this is my first experience with
Autotest, so I'd appreciate close scrutiny.

Finally: I wonder whether it would be better to give up the idea
of atomically replacing the output file at all.  A possibly
simpler approach would be to just unlink the output file if
output is interrupted by a signal (as long as the output file is
a regular file).  This would accomplish the original goal of not
leaving partial output files just as well, but avoid the need for
atomic replacement.  If you like that approach better, then I'll
re-implement it that way.

Thanks,

Ben.

commit 7d431073bc51695bac07cb393ac5b86bc6d9a85b
Author: Ben Pfaff <address@hidden>
Date:   Tue Aug 12 20:41:00 2008 -0700

    * bin/autom4te.in (handle_output): Atomically replace the output
    file, in most circumstances, instead of overwriting it, to avoid
    leaving a partially written output file.

diff --git a/ChangeLog b/ChangeLog
index 4c4cc3a..a481173 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2008-08-11  Ben Pfaff  <address@hidden>
+
+       * bin/autom4te.in (handle_output): Atomically replace the output
+       file, in most circumstances, instead of overwriting it, to avoid
+       leaving a partially written output file.
+
 2008-08-06  Eric Blake  <address@hidden>
 
        Fix autoheader 2.62 regression on AC_DEFINE([__EXTENSIONS__]).
diff --git a/bin/autom4te.in b/bin/autom4te.in
index 685df41..5b96854 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -44,6 +44,7 @@ use Autom4te::FileUtils;
 use Autom4te::General;
 use Autom4te::XFile;
 use File::Basename;
+use File::Copy;
 use strict;
 
 # Data directory.
@@ -485,7 +486,6 @@ sub handle_m4 ($@)
   # Everything went ok: preserve the outputs.
   foreach my $file (map { $_ . $req->id } ($tcache, $ocache))
     {
-      use File::Copy;
       move ("${file}t", "$file")
        or fatal "cannot rename ${file}t as $file: $!";
     }
@@ -550,21 +550,45 @@ sub handle_output ($$)
     foreach (sort keys %forbidden);
   verb "allowed   tokens: $allowed";
 
-  # Read the (cached) raw M4 output, produce the actual result.  We
-  # have to use the 2nd arg to have Autom4te::XFile honor the third, but then
-  # stdout is to be handled by hand :(.  Don't use fdopen as it means
-  # we will close STDOUT, which we already do in END.
+  # Create temporary file to receive output, so that the new output
+  # can atomically replace the old output, or overwrite the old output
+  # file if for some reason that will not work.
   my $out = new Autom4te::XFile;
-  if ($output eq '-')
+  my $tmp_output = "$output.tmp$$";
+  my $atomic_replace;
+  if ($output eq '-' || (-e $output && ! -f $output))
     {
       $out->open (">$output");
+      $atomic_replace = 0;
     }
   else
     {
-      $out->open($output, O_CREAT | O_WRONLY | O_TRUNC, oct ($mode));
+      $SIG{'HUP'} = $SIG{'INT'} = $SIG{'PIPE'} = $SIG{'TERM'} =
+       sub {
+         my ($signal) = @_;
+         unlink ($tmp_output);
+         $SIG{$signal} = 'DEFAULT';
+         kill ($signal, $$);
+       };
+
+      $out->open ($tmp_output, O_CREAT | O_WRONLY | O_TRUNC, oct ($mode));
+      if ($out)
+       {
+         $atomic_replace = 1;
+       }
+      else
+       {
+         $out->open ($output, O_CREAT | O_WRONLY | O_TRUNC, oct ($mode));
+         $atomic_replace = 0;
+       }
     }
   fatal "cannot create $output: $!"
     unless $out;
+
+  # Read the (cached) raw M4 output, produce the actual result.  We
+  # have to use the 2nd arg to have Autom4te::XFile honor the third, but then
+  # stdout is to be handled by hand :(.  Don't use fdopen as it means
+  # we will close STDOUT, which we already do in END.
   my $in = new Autom4te::XFile ("< " . open_quote ($ocache . $req->id));
 
   my %prohibited;
@@ -600,6 +624,14 @@ sub handle_output ($$)
 
   $out->close();
 
+  if ($atomic_replace
+      && !rename ($tmp_output, $output)
+      && !move ($tmp_output, $output))
+    {
+      unlink ($tmp_output);
+      fatal "cannot rename $tmp_output as $output: $!";
+    }
+
   # If no forbidden words, we're done.
   return
     if ! %prohibited;
diff --git a/tests/tools.at b/tests/tools.at
index 9e13689..f3b29b6 100644
--- a/tests/tools.at
+++ b/tests/tools.at
@@ -211,6 +211,26 @@ done
 
 AT_CLEANUP
 
+# autom4te atomic replacement of output
+# -------------------------------------
+
+AT_SETUP([autom4te atomic replacement of output])
+
+AT_DATA([file],
+[[right
+]])
+AT_DATA([file.m4],
+[[m4@&address@hidden([kill $PPID])
+wrong
+]])
+AT_CHECK_AUTOM4TE([-o file file.m4])
+
+AT_CHECK([cat file], 0,
+[[right
+]])
+
+AT_CLEANUP
+
 
 ## ------------------ ##
 ## autoconf --trace.  ##

-- 
"The sound of peacocks being shredded can't possibly be
 any worse than the sound of peacocks not being shredded."
Tanuki the Raccoon-dog in the Monastery