fix: Address unbounded memory growth in upload_stream when source outpaces upload - #3407
Conversation
|
Detected 12 possible performance regressions:
|
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
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).
|
Are there plans to complete and merge the fix soon? We are experiencing OOM kills in our production system due to it. |
jterapin
left a comment
There was a problem hiding this comment.
Nice, I approved on a condition that we will need to amend the changelog entries.
Additionally, we should create a new ticket to do a fast-follow on the file downloader as well. (also double check any classes that utilizes executors).
Fixes #3393.
Description
upload_streamheld memory proportional to the whole object when the source wrote faster than parts uploaded. Since #3302, the reader offloads each part intoDefaultExecutor, whose unbounded queue let it read ahead of the upload threads without limit (#3393).IO.copy_streaminto aStringIOgrows the buffer by doubling, so each 5 MB part held an 8 MB string and fragmented the heap.StandardError(NoMemoryError,SystemStackError,LoadError) killed its worker and was left out of the completed parts. On real S3,upload_streamreturned normally and committed a 15 MB object missing part 2 of a 20 MB source.upload_filewas protected, because S3 rejects its incomplete completion throughmpu_object_size.upload_streamand ranged downloads hung.upload_streamand leftupload_file's multipart upload in progress with no abort.Fix
DefaultExecutoraccepts an optionalmax_queue, andupload_streamsets it tothread_count, sopostblocks while the queue is full.2 * thread_count + 1parts. Peak RSS is about 110 MB regardless of source size, with no change in elapsed time. The tempfile path keepscopy_stream.shutdownandkillclose the queue instead of pushing sentinels, andpostpushes outside the mutex, so a producer waiting on a full queue can't delay them. Without this,shutdown(1)andkilleach took 10 s with a bounded queue.DefaultExecutor::RejectedExecutionError, aRuntimeErrorsubclass, and both uploaders abort the multipart upload when a post is rejected.MultipartUploadErrorinstead of completing without the part.shutdownkeeps joining until the pool is empty, ignores errors re-raised by joining a dead worker, then re-raises the first fatal error once all tasks finish.:thread_count. The sameFileDownloaderissues (a zero-filled hole at the destination on a fatal part error, and rejected-post handling) predate this PR and will be fixed in a follow-up.Testing
aws-sdk-s31681 examples, 0 failures. That includesdefault_executor12,multipart_stream_uploader19,multipart_file_uploader11 andobject/upload_stream5. Each new spec fails with its fix reverted. Repro scripts were run againstversion-3and this branch for memory, rejected posts, worker death and non-StandardErrorpart failures. The missing-part case was also checked on real S3.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!