Address feedback for FileUploader and MultipartFileUploader

Author: jterapin

gems/aws-sdk-s3/lib/aws-sdk-s3/customizations/object.rb

@@ -459,11 +459,17 @@ def upload_stream(options = {}, &block)
       # @see Client#upload_part
       def upload_file(source, options = {})
         uploading_options = options.dup
-        uploader = FileUploader.new(multipart_threshold: uploading_options.delete(:multipart_threshold), client: client)
+        executor = DefaultExecutor.new(max_threads: uploading_options.delete(:thread_count))
+        uploader = FileUploader.new(
+          client: client,
+          executor: executor,
+          multipart_threshold: uploading_options.delete(:multipart_threshold)
+        )
         response = Aws::Plugins::UserAgent.metric('RESOURCE_MODEL') do
           uploader.upload(source, uploading_options.merge(bucket: bucket_name, key: key))
         end
         yield response if block_given?
+        executor.shutdown
         true
       end
       deprecated(:upload_file, use: 'Aws::S3::TransferManager#upload_file', version: 'next major version')
@@ -543,8 +549,8 @@ def download_file(destination, options = {})
         downloader = FileDownloader.new(client: client, executor: executor)
         Aws::Plugins::UserAgent.metric('RESOURCE_MODEL') do
           downloader.download(destination, options.merge(bucket: bucket_name, key: key))
-          executor.shutdown
         end
+        executor.shutdown
         true
       end
       deprecated(:download_file, use: 'Aws::S3::TransferManager#download_file', version: 'next major version')

gems/aws-sdk-s3/lib/aws-sdk-s3/file_uploader.rb

@@ -13,8 +13,8 @@ class FileUploader
       # @option options [Client] :client
       # @option options [Integer] :multipart_threshold (104857600)
       def initialize(options = {})
-        @options = options
         @client = options[:client] || Client.new
+        @executor = options[:executor]
         @multipart_threshold = options[:multipart_threshold] || DEFAULT_MULTIPART_THRESHOLD
       end
 
@@ -36,11 +36,9 @@ def initialize(options = {})
       # @return [void]
       def upload(source, options = {})
         Aws::Plugins::UserAgent.metric('S3_TRANSFER') do
-          if File.size(source) >= multipart_threshold
-            MultipartFileUploader.new(@options).upload(source, options)
+          if File.size(source) >= @multipart_threshold
+            MultipartFileUploader.new(client: @client, executor: @executor).upload(source, options)
           else
-            # remove multipart parameters not supported by put_object
-            options.delete(:thread_count)
             put_object(source, options)
           end
         end

gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_file_uploader.rb

@@ -20,12 +20,9 @@ class MultipartFileUploader
       )
 
       # @option options [Client] :client
-      # @option options [Integer] :thread_count (DEFAULT_THREAD_COUNT)
       def initialize(options = {})
         @client = options[:client] || Client.new
-        @thread_count = options[:thread_count] || DEFAULT_THREAD_COUNT
-        @executor = options[:executor] || DefaultExecutor.new(max_threads: @thread_count)
-        @options = options
+        @executor = options[:executor]
       end
 
       # @return [Client]
@@ -45,8 +42,6 @@ def upload(source, options = {})
         upload_id = initiate_upload(options)
         parts = upload_parts(upload_id, source, file_size, options)
         complete_upload(upload_id, parts, file_size, options)
-      ensure
-        @executor.shutdown if @executor.running? && @options[:executor].nil?
       end
 
       private
@@ -115,19 +110,19 @@ def checksum_key?(key)
         CHECKSUM_KEYS.include?(key)
       end
 
-      def checksum_keys?(keys)
+      def has_checksum_keys?(keys)
         keys.any? { |key| checksum_key?(key) }
       end
 
       def create_opts(options)
         opts = { checksum_algorithm: Aws::Plugins::ChecksumAlgorithm::DEFAULT_CHECKSUM }
-        opts[:checksum_type] = 'FULL_OBJECT' if checksum_keys?(options.keys)
+        opts[:checksum_type] = 'FULL_OBJECT' if has_checksum_keys?(options.keys)
         CREATE_OPTIONS.each_with_object(opts) { |k, h| h[k] = options[k] if options.key?(k) }
       end
 
       def complete_opts(options)
         opts = {}
-        opts[:checksum_type] = 'FULL_OBJECT' if checksum_keys?(options.keys)
+        opts[:checksum_type] = 'FULL_OBJECT' if has_checksum_keys?(options.keys)
         COMPLETE_OPTIONS.each_with_object(opts) { |k, h| h[k] = options[k] if options.key?(k) }
       end
 
@@ -173,8 +168,8 @@ def upload_with_executor(pending, completed, options)
         errors
       end
 
-      def compute_default_part_size(source_size)
-        [(source_size.to_f / MAX_PARTS).ceil, MIN_PART_SIZE].max.to_i
+      def compute_default_part_size(file_size)
+        [(file_size.to_f / MAX_PARTS).ceil, MIN_PART_SIZE].max.to_i
       end
 
       def part_size(total_size, part_size, offset)

gems/aws-sdk-s3/spec/file_uploader_spec.rb

@@ -81,12 +81,6 @@ module S3
 
           subject.upload(ten_meg_file.path, params)
         end
-
-        it 'does not fail when given :thread_count' do
-          expect(client).to receive(:put_object).with(params.merge(body: ten_meg_file))
-
-          subject.upload(ten_meg_file, params.merge(thread_count: 1))
-        end
       end
     end
   end

gems/aws-sdk-s3/spec/multipart_file_uploader_spec.rb

@@ -7,7 +7,8 @@ module Aws
   module S3
     describe MultipartFileUploader do
       let(:client) { S3::Client.new(stub_responses: true) }
-      let(:subject) { MultipartFileUploader.new(client: client) }
+      let(:executor) { DefaultExecutor.new }
+      let(:subject) { MultipartFileUploader.new(client: client, executor: executor) }
       let(:params) { { bucket: 'bucket', key: 'key' } }
 
       describe '#initialize' do