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 6 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.
jterapin
left a comment
jterapin
left a comment
There was a problem hiding this comment.
The overall approach makes sense. I would like to see a fuller PR description for historical purposes (aside from the issue link).
| @max_queue = options[:max_queue] || 0 | ||
| @state = RUNNING | ||
| @queue = Queue.new | ||
| # A bounded queue applies backpressure to producers when full. 0 means unbounded. |
There was a problem hiding this comment.
Could remove this comment. Seems self-explanatory.
| thread_count = upload_opts.delete(:thread_count) || DefaultExecutor::DEFAULT_MAX_THREADS | ||
| # A bounded queue prevents the source from reading ahead without limit when it | ||
| # produces data faster than parts can be uploaded. | ||
| executor = DefaultExecutor.new(max_threads: thread_count, max_queue: thread_count) |
There was a problem hiding this comment.
What's the rationale for keeping the queue size same as thread count?
| if bytes_copied.zero? | ||
| if temp_io.is_a?(Tempfile) | ||
| if @tempfile | ||
| temp_io = Tempfile.new('aws-sdk-s3-upload_stream') |
There was a problem hiding this comment.
This just occurred to me. Is it important to make the file name unique or no?
| end | ||
| # Pushed outside the mutex because a bounded queue blocks the caller when | ||
| # full and holding the lock while parked would deadlock #shutdown and #kill. | ||
| @queue.push([args, block]) |
There was a problem hiding this comment.
I have some thoughts on this but i think offline discussion is better for this. Will DM you.
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!