Merge branch 'master' into codex/coerce-file-path

Author: nobu

.github/workflows/push_gem.yml

@@ -36,7 +36,7 @@ jobs:
           ruby-version: ruby
 
       - name: Publish to RubyGems
-        uses: rubygems/release-gem@052cc82692552de3ef2b81fd670e41d13cba8092 # v1.4.0
+        uses: rubygems/release-gem@7f9650160c1a4e7989fdc9855807bdbd421d8b6b # v1.4.1
 
       - name: Create GitHub release
         run: |

lib/pstore.rb

@@ -389,7 +389,7 @@ def initialize(file, thread_safe = false)
 
   # Raises PStore::Error if the calling code is not in a PStore#transaction.
   def in_transaction
-    raise PStore::Error, "not in transaction" unless @lock.locked?
+    raise PStore::Error, "not in transaction" unless @lock.owned?
   end
   #
   # Raises PStore::Error if the calling code is not in a PStore#transaction or
@@ -439,7 +439,7 @@ def [](key)
   def fetch(key, default=PStore::Error)
     in_transaction
     unless @table.key? key
-      if default == PStore::Error
+      if PStore::Error.equal?(default)
         raise PStore::Error, format("undefined key '%s'", key)
       else
         return default
@@ -527,7 +527,7 @@ def path
   def commit
     in_transaction
     @abort = false
-    throw :pstore_abort_transaction
+    throw self
   end
 
   # Exits the current transaction block, discarding any changes
@@ -538,7 +538,7 @@ def commit
   def abort
     in_transaction
     @abort = true
-    throw :pstore_abort_transaction
+    throw self
   end
 
   # Opens a transaction block for the store.
@@ -570,7 +570,7 @@ def transaction(read_only = false)  # :yields:  pstore
         begin
           @table, checksum, original_data_size = load_data(file, read_only)
 
-          catch(:pstore_abort_transaction) do
+          catch(self) do
             value = yield(self)
           end
 
@@ -583,7 +583,7 @@ def transaction(read_only = false)  # :yields:  pstore
       else
         # This can only occur if read_only == true.
         @table = {}
-        catch(:pstore_abort_transaction) do
+        catch(self) do
           value = yield(self)
         end
       end
@@ -621,23 +621,26 @@ def transaction(read_only = false)  # :yields:  pstore
   # All exceptions are propagated.
   #
   def open_and_lock_file(filename, read_only)
-    if read_only
-      begin
-        file = File.new(filename, **RD_ACCESS)
+    filename = File.path(filename)
+    loop do
+      if read_only
         begin
-          file.flock(File::LOCK_SH)
-          return file
-        rescue
-          file.close
-          raise
+          file = File.new(filename, **RD_ACCESS)
+        rescue Errno::ENOENT
+          return nil
         end
-      rescue Errno::ENOENT
-        return nil
+      else
+        file = File.new(filename, **RDWR_ACCESS)
+      end
+      current = false
+      begin
+        file.flock(read_only ? File::LOCK_SH : File::LOCK_EX)
+        # An atomic save may have replaced the file while this lock was pending.
+        current = File.identical?(file, filename)
+        return file if current
+      ensure
+        file.close unless current
       end
-    else
-      file = File.new(filename, **RDWR_ACCESS)
-      file.flock(File::LOCK_EX)
-      return file
     end
   end
 
@@ -651,6 +654,7 @@ def load_data(file, read_only)
         table = load(file)
         raise Error, "PStore file seems to be corrupted." unless table.is_a?(Hash)
       rescue EOFError
+        raise unless file.size == 0
         # This seems to be a newly-created file.
         table = {}
       end
@@ -700,7 +704,7 @@ def save_data_with_atomic_file_rename_strategy(data, file)
       temp_file.write(data)
       temp_file.flush
       File.rename(temp_filename, @filename)
-    rescue
+    rescue Exception
       File.unlink(temp_file) rescue nil
       raise
     ensure

test/test_pstore.rb

@@ -191,6 +191,113 @@ def clear_store
   def second_file
     File.join(Dir.tmpdir, "pstore.tmp2.#{Process.pid}")
   end
+
+  def test_truncated_read_only_store_is_not_empty
+    @pstore.transaction { @pstore[:foo] = "bar" }
+    data = File.binread(@pstore_file)
+    File.binwrite(@pstore_file, data.byteslice(0, data.bytesize - 1))
+
+    assert_raise(EOFError) { @pstore.transaction(true) { @pstore[:foo] } }
+  end
+
+  def test_store_operations_require_transaction_owner
+    ready = Thread::Queue.new
+    release = Thread::Queue.new
+    owner = Thread.new do
+      @pstore.transaction do
+        @pstore[:foo] = "bar"
+        ready << true
+        release.pop
+      end
+    end
+
+    ready.pop
+    assert_raise(PStore::Error) { @pstore[:foo] }
+    assert_raise(PStore::Error) { @pstore[:foo] = "other" }
+  ensure
+    release << true if release
+    owner.join if owner
+  end
+
+  def test_commit_targets_the_owning_store
+    inner = PStore.new(second_file)
+    @pstore.transaction do
+      @pstore[:outer] = true
+      inner.transaction do
+        inner[:inner] = true
+        @pstore.commit
+        flunk("outer commit should exit its own transaction")
+      end
+      flunk("outer commit should exit its own transaction")
+    end
+
+    assert_equal(true, @pstore.transaction(true) { @pstore[:outer] })
+    assert_nil(inner.transaction(true) { inner[:inner] })
+  ensure
+    File.unlink(second_file) rescue nil
+  end
+
+  def test_fetch_compares_missing_default_by_identity
+    default = Object.new
+    def default.==(_other)
+      true
+    end
+
+    assert_same(default, @pstore.transaction(true) { @pstore.fetch(:missing, default) })
+  end
+
+  def test_lock_retries_when_atomic_save_replaces_file
+    fake_file = Struct.new(:closed) do
+      def flock(_mode)
+        true
+      end
+
+      def close
+        self.closed = true
+      end
+    end
+    first = fake_file.new(false)
+    second = fake_file.new(false)
+    files = [first, second]
+    original_new = File.method(:new)
+    original_identical = File.method(:identical?)
+    File.define_singleton_method(:new) { |*_args, **_kwargs| files.shift }
+    File.define_singleton_method(:identical?) { |file, _path| file.equal?(second) }
+
+    result = @pstore.send(:open_and_lock_file, @pstore_file, false)
+
+    assert_same(second, result)
+    assert_equal(true, first.closed)
+    assert_equal(false, second.closed)
+  ensure
+    result.close if result && !result.closed
+    File.define_singleton_method(:new, original_new) if original_new
+    File.define_singleton_method(:identical?, original_identical) if original_identical
+  end
+ 
+  def test_interrupted_atomic_save_removes_temporary_file
+    return if /mswin|mingw|bccwin|wince/ =~ RUBY_PLATFORM
+
+    @pstore.transaction { @pstore[:foo] = "old" }
+    @pstore.ultra_safe = true
+    original_new = File.method(:new)
+    store_path = @pstore_file
+    File.define_singleton_method(:new) do |path, **options|
+      file = original_new.call(path, **options)
+      if path.start_with?("#{store_path}.tmp.")
+        file.define_singleton_method(:write) { |_data| raise Interrupt }
+      end
+      file
+    end
+
+    assert_raise(Interrupt) { @pstore.transaction { @pstore[:foo] = "new" } }
+    assert_equal([], Dir.glob("#{@pstore_file}.tmp.*"))
+    assert_equal("old", @pstore.transaction(true) { @pstore[:foo] })
+  ensure
+    File.define_singleton_method(:new, original_new) if original_new
+    Dir.glob("#{@pstore_file}.tmp.*").each { |path| File.unlink(path) rescue nil }
+  end
+
   def test_path_like_filename_is_normalized
     store = PStore.new(Pathname(@pstore_file))
     store.ultra_safe = true
@@ -200,5 +307,4 @@ def test_path_like_filename_is_normalized
     assert_equal(@pstore_file, store.path)
     assert_equal("bar", store.transaction(true) { store[:foo] })
   end
-
 end