Merge pull request #875 from samvera/dont_open_file_descriptors

StorageAdapter#upload_file/StorageAdapter#find_by shouldn't open handles

.circleci/config.yml

@@ -65,7 +65,7 @@ jobs:
       - restore_cache:
          keys:
            - bundle-{{ checksum "<< parameters.gemfile >>" }}-{{ checksum "valkyrie.gemspec" }}-<< parameters.ruby >>-6
-      - run: sudo apt update -y && sudo apt-get install -y libpq-dev
+      - run: sudo apt update -y && sudo apt-get install -y libpq-dev lsof
       - run:
           name: Set BUNDLE_GEMFILE
           command: |

lib/valkyrie/specs/shared_specs/storage_adapter.rb

@@ -30,6 +30,25 @@ class Valkyrie::Specs::CustomResource < Valkyrie::Resource
     expect(uploaded_file.valid?(digests: { sha1: sha1 })).to be true
   end
 
+  it "doesn't leave a file handle open on upload/find_by" do
+    # No file handle left open from upload.
+    resource = Valkyrie::Specs::CustomResource.new(id: "testdiscovery")
+    pre_open_files = open_files
+    uploaded_file = storage_adapter.upload(file: file, original_filename: 'foo.jpg', resource: resource, fake_upload_argument: true)
+    file.close
+    expect(pre_open_files.size).to eq open_files.size
+
+    # No file handle left open from find_by
+    pre_open_files = open_files
+    the_file = storage_adapter.find_by(id: uploaded_file.id)
+    expect(the_file).to be_kind_of Valkyrie::StorageAdapter::File
+    expect(pre_open_files.size).to eq open_files.size
+  end
+
+  def open_files
+    `lsof +D . | awk '{print $9}'`.split.uniq[1..-1]
+  end
+
   it "can upload, validate, re-fetch, and delete a file" do
     resource = Valkyrie::Specs::CustomResource.new(id: "test")
     sha1 = Digest::SHA1.file(file).to_s

lib/valkyrie/storage/disk.rb

@@ -36,11 +36,34 @@ def file_path(id)
     # @return [Valkyrie::StorageAdapter::File]
     # @raise Valkyrie::StorageAdapter::FileNotFound if nothing is found
     def find_by(id:)
-      Valkyrie::StorageAdapter::File.new(id: Valkyrie::ID.new(id.to_s), io: ::File.open(file_path(id), 'rb'))
+      Valkyrie::StorageAdapter::File.new(id: Valkyrie::ID.new(id.to_s), io: LazyFile.open(file_path(id), 'rb'))
     rescue Errno::ENOENT
       raise Valkyrie::StorageAdapter::FileNotFound
     end
 
+    ## LazyFile takes File.open parameters but doesn't leave a file handle open on
+    # instantiation. This way StorageAdapter#find_by doesn't open a handle
+    # silently and never clean up after itself.
+    class LazyFile
+      def self.open(path, mode)
+        # Open the file regularly and close it, so it can error if it doesn't
+        # exist.
+        File.open(path, mode).close
+        new(path, mode)
+      end
+
+      delegate(*(File.instance_methods - Object.instance_methods), to: :_inner_file)
+
+      def initialize(path, mode)
+        @__path = path
+        @__mode = mode
+      end
+
+      def _inner_file
+        @_inner_file ||= File.open(@__path, @__mode)
+      end
+    end
+
     # Delete the file on disk associated with the given identifier.
     # @param id [Valkyrie::ID]
     def delete(id:)