S3 Executor support #3302

jterapin · 2025-10-07T18:35:42Z

This PR introduces a lightweight DefaultExecutor to replace the legacy :thread_count approach, laying the foundation for directory upload/download feature. Additionally, we support the following:

Custom executor support - Customers are able to pass in their own executor implementation (must implement DefaultExecutor interface)
:thread_count configuration is still respected - Existing :thread_count option continues to work by
automatically creating a DefaultExecutor with the specified thread count

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the CHANGELOG.md file (at corresponding gem). For the description entry, please make sure it lives in one line and starts with Feature or Issue in the correct format.
For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md

Thank you for your contribution!

github-actions · 2025-10-07T18:41:32Z

Detected 1 possible performance regressions:

aws-sdk-s3.get_object_small_allocated_kb - z-score regression: 64.25 -> 64.39. Z-score: Infinity

mullermp

Nice so far!

mullermp · 2025-10-09T18:29:59Z

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

+
+module Aws
+  module S3
+    # @api private


Is this private or public?

You know - I was super torn whether I should make this private or not. It is definitely an inner detail but at the same time, it's an interface that customer will have to abide when implementing their own executor. Thoughts?

I guess it's private since thread count option is passed through. However if we haven't already, make clear how to provide a custom one.

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

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

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

mullermp · 2025-10-09T18:43:51Z

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

+        errors = []
+
        if (callback = options[:progress_callback])
          progress = MultipartProgress.new(pending, callback)


You can simplify the usages of these I think by always making a multipart progress object and short circuiting its callback if it doesn't exist.

Let me know if the revision makes it better

mullermp · 2025-10-09T18:51:31Z

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

    # * track transfer progress by using progress listener
    #
    class TransferManager
+      # @example Using default executor (automatic creation and shutdown)


We can consider using at_exit to handle shutdown automatically for all cases, rather than for only the default and having a customer managed executor. We can just require it to implement a shutdown method and we always call it. https://docs.ruby-lang.org/en/3.4/Kernel.html#method-i-at_exit

That's a pretty neat. I'm still a bit worried about custom executors though, especially if customers are reusing that executor elsewhere in their app and have their own at_exit hooks (not sure if this is a valid case). We might run into exit ordering issues that we can't control.

I'm not sure I follow. If they are using a custom executor then we would only shut it down when the program exits. If they shut it down before us in the program or at their own exit hook, our shutdown call happens but becomes no op. I do think you should investigate this option more.

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

mullermp · 2025-10-09T18:53:40Z

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

+        executor = @executor || DefaultExecutor.new(max_threads: download_opts.delete(:thread_count))
+        downloader = FileDownloader.new(client: @client, executor: executor)
        downloader.download(destination, options.merge(bucket: bucket, key: key))
+        executor.shutdown unless @options[:executor]


You could just inspect @executor here and not store all of the options.

This is kind of how I know whether if @executor is custom or not - so that I don't shut down custom executors.

mullermp · 2025-10-09T18:54:27Z

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

+          expect(block).to receive(:call).with('hello')
+
+          subject.post('hello') { |arg| block.call(arg) }
+          sleep 0.1


I'd prefer tests without actual sleeping somehow - these make tests slower.

Fair. I shorten the timing so maybe this will help.
Alternatively, I was thinking of using Thread.pass but I'm bit worried about the global effect of it:

describe '#post' do it 'executes a block with arguments' do result = nil subject.post('hello') { |a| result = a } Thread.pass until result expect(result).to eq('hello') end

I suspect these kinds of tests will be flakey. I think you should find a way to do it with expectations rather than blocking with sleep.

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

mullermp · 2025-10-11T01:27:54Z

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

      # @see Client#head_object
      def download_file(destination, options = {})
-        downloader = FileDownloader.new(client: client)
+        download_options = options.dup


As an optimization, rather than dup, you can just merge bucket and key which creates new hash then you can delete off keys.

mullermp · 2025-10-11T01:28:29Z

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

-        @params = options
-        validate!
+        validate_destination!(destination)
+        opts = build_download_opts(destination, options.dup)


I don't think we care about dup here either because we should be given a new hash already at this point.

mullermp · 2025-10-11T01:35:44Z

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

-            ensure
-              # Ensure the pipe is closed to avoid https://github.com/jruby/jruby/issues/6111
-              write_pipe.close
+            upload_thread = Thread.new do


Well if I set thread count to 10 but 11 threads are used, that's maybe undesirable. Using post maybe makes sense so that it's at most 10 for the entire operation but that's up to you.

mullermp · 2025-10-11T01:37:49Z

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

    # * track transfer progress by using progress listener
    #
    class TransferManager
+      # @example Using default executor (automatic creation and shutdown)


I'm not sure I follow. If they are using a custom executor then we would only shut it down when the program exits. If they shut it down before us in the program or at their own exit hook, our shutdown call happens but becomes no op. I do think you should investigate this option more.

mullermp · 2025-10-11T01:38:31Z

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

      # @see Client#head_object
      def download_file(destination, bucket:, key:, **options)
-        downloader = FileDownloader.new(client: @client)
+        download_opts = options.dup


Ditto about dup comments (applies to other places)

mullermp · 2025-10-11T01:39:55Z

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

+          expect(block).to receive(:call).with('hello')
+
+          subject.post('hello') { |arg| block.call(arg) }
+          sleep 0.1


I suspect these kinds of tests will be flakey. I think you should find a way to do it with expectations rather than blocking with sleep.

mullermp · 2025-10-11T01:41:19Z

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

+        end
+
+        it 'kills threads after timeout' do
+          result = nil


Should you have asserted result is done at some point?

Add executor support

8c7ca45

jterapin added 15 commits October 7, 2025 14:10

Add changelog entry

c21969a

Update TM with executor changes

39ecf0a

Remove thread count support from MPU

a3f2b9f

Update Object usage of executor

3156f7c

Add documentation/remove unused methods from DefaultExecutor

84c9966

Add Default Executor specs

8e16a3b

Update TM docs and impl

db1cb62

Update streaming MPU to use executor

f907c3b

More MP Stream updates

7cb940a

Update specs

4003536

Update interfaces

7dddda9

Update specs

481f198

Update changelog

88bf44a

Minor updates

c1a25cd

Fix failing specs

7522a16

jterapin marked this pull request as ready for review October 9, 2025 18:11

mullermp reviewed Oct 9, 2025

View reviewed changes

jterapin added 7 commits October 10, 2025 08:22

Merge branch 'version-3' into s3-executor-support

89cffe7

Feedback - address sleep in specs

9eea233

Feedback - update method name for cleanup_team_file

75b0d96

Feedback - wrap checksum callback

ad943ee

Feedback - update method name in MPU

f1fc86a

Feedback - streamline handling of progress callbacks

09eae68

Feedback - streamline docs

e824de0

jterapin requested a review from mullermp October 10, 2025 19:17

mullermp approved these changes Oct 11, 2025

View reviewed changes

S3 Executor support #3302

Are you sure you want to change the base?

S3 Executor support #3302

Uh oh!

Conversation

jterapin commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mullermp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jterapin commented Oct 7, 2025 •

edited

Loading

github-actions bot commented Oct 7, 2025 •

edited

Loading