fix: Address unbounded memory growth in upload_stream when source outpaces upload#3407
fix: Address unbounded memory growth in upload_stream when source outpaces upload#3407richardwang1124 wants to merge 4 commits into
Conversation
|
Detected 1 possible performance regressions:
|
jterapin
left a comment
jterapin
left a comment
There was a problem hiding this comment.
Thanks for picking this up! this is a real gap and a good catch. I've left a few questions on the current implementation inline. We're also missing a CHANGELOG entry, so we'll want to add one before this merges.
| @@ -12,7 +12,7 @@ class DefaultExecutor | |||
| def initialize(options = {}) | |||
| @max_threads = options[:max_threads] || DEFAULT_MAX_THREADS | |||
| @state = RUNNING | |||
| @queue = Queue.new | |||
| @queue = SizedQueue.new(@max_threads) | |||
There was a problem hiding this comment.
I think its worthwhile to offer @max_queue as a separate option and pass in a reasonable default to :max_queue when creating an executor within the scope of the streaming uploader.
| ensure_worker_available | ||
| end | ||
| @queue << [args, block] |
There was a problem hiding this comment.
You moved the enqueue out of the mutex to avoid holding the lock while parked but what happens shutdown or kill is called? There is also a gap between @state check and the push: what happens when shutdown/kill slips into that gap?
| temp_io.rewind | ||
| if bytes_copied.zero? | ||
| if temp_io.is_a?(Tempfile) | ||
| if @tempfile |
There was a problem hiding this comment.
For my own understanding: What's the reason for the split here? The tempfile branch needs copy_stream (streams to disk, no heap copy), but the old code used copy_stream for the in-memory path too...was there a reason to switch it to read?
2 participants
Fixes #3393.
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.mdfile (at corresponding gem). For the description entry, please make sure it lives in one line and starts withFeatureorIssuein 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!