-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Address unbounded memory growth in upload_stream when source outpaces upload #3407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: version-3
Are you sure you want to change the base?
Changes from all commits
8080821
842f7e7
906c099
c796bc5
b20cd27
e1d7bf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,10 @@ class DefaultExecutor | |
|
|
||
| def initialize(options = {}) | ||
| @max_threads = options[:max_threads] || DEFAULT_MAX_THREADS | ||
| @max_queue = options[:max_queue] || 0 | ||
| @state = RUNNING | ||
| @queue = Queue.new | ||
| # A bounded queue applies backpressure to producers when full. 0 means unbounded. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could remove this comment. Seems self-explanatory. |
||
| @queue = @max_queue.zero? ? Queue.new : SizedQueue.new(@max_queue) | ||
| @pool = [] | ||
| @mutex = Mutex.new | ||
| end | ||
|
|
@@ -25,10 +27,15 @@ def post(*args, &block) | |
| @mutex.synchronize do | ||
| raise 'Executor has been shutdown and is no longer accepting tasks' unless @state == RUNNING | ||
|
|
||
| @queue << [args, block] | ||
| ensure_worker_available | ||
| 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]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have some thoughts on this but i think offline discussion is better for this. Will DM you. |
||
| true | ||
| rescue ClosedQueueError | ||
| # shutdown or kill happened while parked on a full queue | ||
| raise 'Executor has been shutdown and is no longer accepting tasks' | ||
| end | ||
|
|
||
| # Immediately terminates all worker threads and clears pending tasks. | ||
|
|
@@ -38,6 +45,7 @@ def post(*args, &block) | |
| def kill | ||
| @mutex.synchronize do | ||
| @state = SHUTDOWN | ||
| @queue.close # wakes any producer parked on a full queue | ||
| @pool.each(&:kill) | ||
| @pool.clear | ||
| @queue.clear | ||
|
|
@@ -56,7 +64,9 @@ def shutdown(timeout = nil) | |
| return true if @state == SHUTDOWN | ||
|
|
||
| @state = SHUTTING_DOWN | ||
| @pool.size.times { @queue << :shutdown } | ||
| # Closing wakes parked producers and lets workers drain remaining tasks | ||
| # before exiting without pushing sentinels onto a queue that may be full. | ||
| @queue.close | ||
| end | ||
|
|
||
| if timeout | ||
|
|
@@ -91,8 +101,6 @@ def ensure_worker_available | |
| def spawn_worker | ||
| Thread.new do | ||
| while (job = @queue.shift) | ||
| break if job == :shutdown | ||
|
|
||
| args, block = job | ||
| block.call(*args) | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,18 +113,24 @@ def complete_opts(options) | |
| def read_to_part_body(read_pipe) | ||
| return if read_pipe.closed? | ||
|
|
||
| temp_io = @tempfile ? Tempfile.new('aws-sdk-s3-upload_stream') : StringIO.new(String.new) | ||
| temp_io.binmode | ||
| bytes_copied = IO.copy_stream(read_pipe, temp_io, @part_size) | ||
| temp_io.rewind | ||
| if bytes_copied.zero? | ||
| if temp_io.is_a?(Tempfile) | ||
| if @tempfile | ||
|
jterapin marked this conversation as resolved.
|
||
| temp_io = Tempfile.new('aws-sdk-s3-upload_stream') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This just occurred to me. Is it important to make the file name unique or no? |
||
| temp_io.binmode | ||
| bytes_copied = IO.copy_stream(read_pipe, temp_io, @part_size) | ||
| temp_io.rewind | ||
| if bytes_copied.zero? | ||
| temp_io.close | ||
| temp_io.unlink | ||
| nil | ||
| else | ||
| temp_io | ||
| end | ||
| nil | ||
| else | ||
| temp_io | ||
| # Read into a single right-sized buffer. IO.copy_stream into a StringIO grows | ||
| # the backing string geometrically (an 8MB buffer for a 5MB part) and discards | ||
| # the intermediates, fragmenting the heap across concurrent parts. | ||
| data = read_pipe.read(@part_size) | ||
| data.nil? ? nil : StringIO.new(data) | ||
| end | ||
| end | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for keeping the queue size same as thread count?