Kaynağa Gözat

Land FlatFile post writes by rename as well as under a lock

fe2c0ce took the lock and left the write in place, which fixed the lost
update and not the torn read.  Both are real, and they want different
answers, so this does both.

The lock stays: File::Slurper::Temp does no locking -- it writes a scratch
file and renames -- so on its own it does not stop two workers each reading N
revisions and each writing back N+1.  Measured over five runs at three
workers by 25 saves, plain open kept 28-38 of 75 revisions and Temp:: kept
25-34, which is to say no better.  Under the lock, 75 every time.

The rename is new, and is what fe2c0ce missed.  Every reader here takes no
lock, so a truncate-and-write in place lets one catch a half-written file:
523 unparseable reads in 160952 against an in-place writer, none at all
against one that renames.

Two things the rename needs to be told.  FILE_TEMP_DIR, because File::Temp
otherwise puts the scratch file next to its target -- inside $datastore,
which _index() scans with readdir, and 1373 of 611329 scans duly saw it.  It
now sits beside the datastore with the lock.  And FILE_TEMP_PERMS, because
File::Temp creates a new file 0600 where the open it replaces honoured the
umask; existing posts are 0644 and new ones would not have been.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CJ2NAMQyytncwHKSmD5dFe
George Baugh 1 ay önce
ebeveyn
işleme
2033b2211a
1 değiştirilmiş dosya ile 38 ekleme ve 23 silme
  1. 38 23
      lib/Trog/Data/FlatFile.pm

+ 38 - 23
lib/Trog/Data/FlatFile.pm

@@ -3,10 +3,13 @@ package Trog::Data::FlatFile;
 use v5.36;
 use re '/aa';
 
-use Carp  qw{confess};
-use Fcntl qw{LOCK_EX SEEK_SET};
+use Carp           qw{confess};
+use Fcntl          qw{LOCK_EX};
+use File::Basename ();
+use File::Path     ();
 use JSON::MaybeXS;
 use File::Slurper;
+use File::Slurper::Temp();
 use File::Copy;
 use Path::Tiny();
 use Capture::Tiny qw{capture_merged};
@@ -111,28 +114,40 @@ sub aliases ($self) {
 }
 
 sub write ( $self, $data ) {
+
+    # The lock and the scratch file both live beside the datastore rather than
+    # in it: _index() scans $datastore with readdir, and would otherwise hand
+    # either of them to the JSON parser as though it were a post.
+    my $spool = File::Basename::dirname($datastore);
+
     foreach my $post (@$data) {
-        my $file   = "$datastore/$post->{id}";
-        my $update = [$post];
-
-        mkdir $datastore;
-
-        # One locked handle for the whole read-modify-write.  This used to be a
-        # -f, then a separate read, then a separate truncating open; two workers
-        # saving the same post could both read the existing revisions and then
-        # each write back only their own, silently dropping the post's history.
-        # '+>>' creates the file if it is not there, and never truncates.
-        open( my $fh, '+>>', $file ) or confess "Could not open $file: $!";
-        flock( $fh, LOCK_EX )        or confess "Could not lock $file: $!";
-
-        seek( $fh, 0, SEEK_SET ) or confess "Could not rewind $file: $!";
-        my $slurped = do { local $/; <$fh> };
-        $update = [ ( @{ $parser->decode($slurped) }, $post ) ] if length( $slurped // '' );
-
-        seek( $fh, 0, SEEK_SET ) or confess "Could not rewind $file: $!";
-        truncate( $fh, 0 )       or confess "Could not truncate $file: $!";
-        print {$fh} $parser->encode($update);
-        close $fh;
+        my $file = "$datastore/$post->{id}";
+
+        File::Path::make_path($datastore);
+
+        # Two separate races here, needing two separate answers.
+        #
+        # The lock is what stops a lost update.  Without it two workers saving
+        # the same post both read the existing N revisions and each write back
+        # N+1, so one save vanishes: at three workers by 25 saves, about 30 of
+        # 75 revisions survived.  With it, all 75.  It has to be taken on a
+        # path that never gets renamed, hence a lockfile rather than the post.
+        open( my $lock, '>>', "$spool/.write.lock" ) or confess "Could not open $spool/.write.lock: $!";
+        flock( $lock, LOCK_EX )                      or confess "Could not lock $spool/.write.lock: $!";
+
+        my $slurped = eval { File::Slurper::read_binary($file) };
+        my $update  = length( $slurped // '' ) ? [ ( @{ $parser->decode($slurped) }, $post ) ] : [$post];
+
+        # The rename is what stops a torn read.  Every reader here takes no
+        # lock, so writing in place lets one catch a half-written file: a few
+        # hundred unparseable reads per hundred thousand, against none once the
+        # write lands by rename.  PERMS because File::Temp would otherwise
+        # create a new post 0600, where a plain open honoured the umask.
+        local $File::Slurper::Temp::FILE_TEMP_DIR   = $spool;
+        local $File::Slurper::Temp::FILE_TEMP_PERMS = oct('666') & ~umask;
+        File::Slurper::Temp::write_binary( $file, $parser->encode($update) );
+
+        close $lock;
 
         Trog::SQLite::TagIndex::add_post( $post, $self );
     }