Merge pull request #2314 from dosuken123/fix-large-file-memeory-consu…

…mption

Fix CarrierWave reads large files into memory

lib/carrierwave/storage/fog.rb

@@ -276,6 +276,16 @@ def initialize(uploader, base, path)
         #
         # [String] contents of file
         def read
+          file_body = file.body
+
+          return if file_body.nil?
+          return file_body unless file_body.is_a?(::File)
+
+          # Fog::Storage::XXX::File#body could return the source file which was upoloaded to the remote server.
+          read_source_file(file_body) if ::File.exist?(file_body.path)
+
+          # If the source file doesn't exist, the remote content is read
+          @file = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables
           file.body
         end
 
@@ -313,7 +323,7 @@ def store(new_file)
             fog_file = new_file.to_file
             @content_type ||= new_file.content_type
             @file = directory.files.create({
-              :body         => (fog_file ? fog_file : new_file).read,
+              :body         => fog_file ? fog_file : new_file.read,
               :content_type => @content_type,
               :key          => path,
               :public       => @uploader.fog_public
@@ -458,6 +468,17 @@ def file
         def acl_header
           {'x-amz-acl' => @uploader.fog_public ? 'public-read' : 'private'}
         end
+
+        def read_source_file(file_body)
+          return unless ::File.exist?(file_body.path)
+
+          begin
+            file_body = ::File.open(file_body.path) if file_body.closed? # Reopen if it's already closed
+            file_body.read
+          ensure
+            file_body.close
+          end
+        end
       end
 
     end # Fog

spec/storage/fog_helper.rb

@@ -45,174 +45,197 @@ class FogSpec#{fog_credentials[:provider]}Uploader < CarrierWave::Uploader::Base
       end
 
       describe '#store!' do
-
         let(:store_path) { 'uploads/test+.jpg' }
 
-        before do
-          allow(@uploader).to receive(:store_path).and_return(store_path)
-          @fog_file = @storage.store!(file)
-        end
+        context 'when file is ::File' do
+          before do
+            allow(@uploader).to receive(:store_path).and_return(store_path)
+            @fog_file = @storage.store!(file)
+          end
 
-        it "should upload the file" do
-          # reading the file after upload should return the body, not a closed tempfile
-          expect(@fog_file.read).to eq('this is stuff')
-          # make sure it actually uploaded to the service, too
-          expect(@directory.files.get(store_path).body).to eq('this is stuff')
-        end
+          it "should upload the file" do
+            # reading the file after upload should return the body, not a closed tempfile
+            expect(@fog_file.read).to eq('this is stuff')
+            # make sure it actually uploaded to the service, too
+            expect(@directory.files.get(store_path).body).to eq('this is stuff')
+          end
 
-        it "should have a path" do
-          expect(@fog_file.path).to eq(store_path)
-        end
+          it "should have a path" do
+            expect(@fog_file.path).to eq(store_path)
+          end
 
-        it "should have a content_type" do
-          expect(@fog_file.content_type).to eq(file.content_type)
-          expect(@directory.files.get(store_path).content_type).to eq(file.content_type)
-        end
+          it "should have a content_type" do
+            expect(@fog_file.content_type).to eq(file.content_type)
+            expect(@directory.files.get(store_path).content_type).to eq(file.content_type)
+          end
 
-        it "should have an extension" do
-          expect(@fog_file.extension).to eq("jpg")
-        end
+          it "should have an extension" do
+            expect(@fog_file.extension).to eq("jpg")
+          end
 
-        context "without asset_host" do
-          it "should have a public_url" do
-            unless fog_credentials[:provider] == 'Local'
-              expect(@fog_file.public_url).not_to be_nil
+          context "without asset_host" do
+            it "should have a public_url" do
+              unless fog_credentials[:provider] == 'Local'
+                expect(@fog_file.public_url).not_to be_nil
+              end
             end
-          end
 
-          it "should have a url" do
-            unless fog_credentials[:provider] == 'Local'
-              expect(@fog_file.url).not_to be_nil
+            it "should have a url" do
+              unless fog_credentials[:provider] == 'Local'
+                expect(@fog_file.url).not_to be_nil
+              end
             end
-          end
 
-          context "directory is a valid subdomain" do
-            before do
-              allow(@uploader).to receive(:fog_directory).and_return('assets.site.com')
+            context "directory is a valid subdomain" do
+              before do
+                allow(@uploader).to receive(:fog_directory).and_return('assets.site.com')
+              end
+
+              it "should use a subdomain URL for AWS" do
+                if @provider == 'AWS'
+                  expect(@fog_file.public_url).to include('https://assets.site.com.s3.amazonaws.com')
+                end
+              end
+
+              it "should use accelerate domain if fog_aws_accelerate is true" do
+                if @provider == 'AWS'
+                  allow(@uploader).to receive(:fog_aws_accelerate).and_return(true)
+                  expect(@fog_file.public_url).to include('https://assets.site.com.s3-accelerate.amazonaws.com')
+                end
+              end
             end
 
-            it "should use a subdomain URL for AWS" do
+            it "should not use a subdomain URL for AWS if the directory is not a valid subdomain" do
               if @provider == 'AWS'
-                expect(@fog_file.public_url).to include('https://assets.site.com.s3.amazonaws.com')
+                allow(@uploader).to receive(:fog_directory).and_return('SiteAssets')
+                expect(@fog_file.public_url).to include('https://s3.amazonaws.com/SiteAssets')
               end
             end
 
-            it "should use accelerate domain if fog_aws_accelerate is true" do
+            it "should use https as a default protocol" do
               if @provider == 'AWS'
-                allow(@uploader).to receive(:fog_aws_accelerate).and_return(true)
-                expect(@fog_file.public_url).to include('https://assets.site.com.s3-accelerate.amazonaws.com')
+                expect(@fog_file.public_url).to start_with 'https://'
               end
             end
-          end
 
-          it "should not use a subdomain URL for AWS if the directory is not a valid subdomain" do
-            if @provider == 'AWS'
-              allow(@uploader).to receive(:fog_directory).and_return('SiteAssets')
-              expect(@fog_file.public_url).to include('https://s3.amazonaws.com/SiteAssets')
+            it "should use https as a default protocol" do
+              if @provider == 'AWS'
+                allow(@uploader).to receive(:fog_use_ssl_for_aws).and_return(false)
+                expect(@fog_file.public_url).to start_with 'http://'
+              end
             end
-          end
 
-          it "should use https as a default protocol" do
-            if @provider == 'AWS'
-              expect(@fog_file.public_url).to start_with 'https://'
+            it "should use the google public url if available" do
+              if @provider == 'Google'
+                allow(@uploader).to receive(:fog_directory).and_return('SiteAssets')
+                expect(@fog_file.public_url).to include('https://storage.googleapis.com/SiteAssets')
+              end
             end
           end
 
-          it "should use https as a default protocol" do
-            if @provider == 'AWS'
-              allow(@uploader).to receive(:fog_use_ssl_for_aws).and_return(false)
-              expect(@fog_file.public_url).to start_with 'http://'
-            end
-          end
+          context "with asset_host" do
+            before { allow(@uploader).to receive(:asset_host).and_return(asset_host) }
 
-          it "should use the google public url if available" do
-            if @provider == 'Google'
-              allow(@uploader).to receive(:fog_directory).and_return('SiteAssets')
-              expect(@fog_file.public_url).to include('https://storage.googleapis.com/SiteAssets')
-            end
-          end
-        end
+            context "when a asset_host is a proc" do
 
-        context "with asset_host" do
-          before { allow(@uploader).to receive(:asset_host).and_return(asset_host) }
+              let(:asset_host) { proc { "http://foo.bar" } }
 
-          context "when a asset_host is a proc" do
+              describe "args passed to proc" do
+                let(:asset_host) { proc { |storage| expect(storage).to be_instance_of ::CarrierWave::Storage::Fog::File } }
 
-            let(:asset_host) { proc { "http://foo.bar" } }
+                it "should be the uploader" do
+                  @fog_file.public_url
+                end
+              end
 
-            describe "args passed to proc" do
-              let(:asset_host) { proc { |storage| expect(storage).to be_instance_of ::CarrierWave::Storage::Fog::File } }
+              it "should have a asset_host rooted public_url" do
+                expect(@fog_file.public_url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              end
 
-              it "should be the uploader" do
-                @fog_file.public_url
+              it "should have a asset_host rooted url" do
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
               end
-            end
 
-            it "should have a asset_host rooted public_url" do
-              expect(@fog_file.public_url).to eq('http://foo.bar/uploads/test%2B.jpg')
-            end
+              it "should always have the same asset_host rooted url" do
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              end
 
-            it "should have a asset_host rooted url" do
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              it 'should retrieve file name' do
+                expect(@fog_file.filename).to eq('test+.jpg')
+              end
             end
 
-            it "should always have the same asset_host rooted url" do
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
-            end
+            context "when a string" do
+              let(:asset_host) { "http://foo.bar" }
+
+              it "should have a asset_host rooted public_url" do
+                expect(@fog_file.public_url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              end
+
+              it "should have a asset_host rooted url" do
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              end
 
-            it 'should retrieve file name' do
-              expect(@fog_file.filename).to eq('test+.jpg')
+              it "should always have the same asset_host rooted url" do
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+                expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+              end
             end
+
           end
 
-          context "when a string" do
-            let(:asset_host) { "http://foo.bar" }
+          context "without extension" do
 
-            it "should have a asset_host rooted public_url" do
-              expect(@fog_file.public_url).to eq('http://foo.bar/uploads/test%2B.jpg')
-            end
+            let(:store_path) { 'uploads/test' }
 
-            it "should have a asset_host rooted url" do
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
+            it "should have no extension" do
+              expect(@fog_file.extension).to be_nil
             end
 
-            it "should always have the same asset_host rooted url" do
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
-              expect(@fog_file.url).to eq('http://foo.bar/uploads/test%2B.jpg')
-            end
           end
 
-        end
-
-        context "without extension" do
-
-          let(:store_path) { 'uploads/test' }
+          it "should return filesize" do
+            expect(@fog_file.size).to eq(13)
+          end
 
-          it "should have no extension" do
-            expect(@fog_file.extension).to be_nil
+          it "should be deletable" do
+            @fog_file.delete
+            expect(@directory.files.head(store_path)).to eq(nil)
           end
 
-        end
+          context "when the file has been deleted" do
+            before { @fog_file.delete }
 
-        it "should return filesize" do
-          expect(@fog_file.size).to eq(13)
-        end
+            it "should not error getting the file size" do
+              expect { @fog_file.size }.not_to raise_error
+            end
 
-        it "should be deletable" do
-          @fog_file.delete
-          expect(@directory.files.head(store_path)).to eq(nil)
+            it "should not error getting the content type" do
+              expect { @fog_file.content_type }.not_to raise_error
+            end
+          end
         end
 
-        context "when the file has been deleted" do
-          before { @fog_file.delete }
+        context 'when file is ::StringIO' do
+          let(:file) do
+            CarrierWave::SanitizedFile.new(
+              :tempfile => StringIO.new('Test StringIO texts'),
+              :filename => 'test.jpg',
+              :content_type => 'image/jpeg'
+            )
+          end
 
-          it "should not error getting the file size" do
-            expect { @fog_file.size }.not_to raise_error
+          before do
+            allow(@uploader).to receive(:store_path).and_return(store_path)
+            @fog_file = @storage.store!(file)
           end
 
-          it "should not error getting the content type" do
-            expect { @fog_file.content_type }.not_to raise_error
+          it "should upload the file" do
+            # reading the file after upload should return the body, not a closed tempfile
+            expect(@fog_file.read).to eq('Test StringIO texts')
+            # make sure it actually uploaded to the service, too
+            expect(@directory.files.get(store_path).body).to eq('Test StringIO texts')
           end
         end
       end