large file is going to blow memory #1780
Labels
Comments
philister
added a commit
to pictrs/carrierwave
that referenced
this issue
Nov 24, 2015
|
Hi @philister, can you provide an example of an uploader that exhibits this issue? |
|
Gemfile.lock (excerpt) animal.rb rails c |
|
@philister Can you provide a full stack trace please? |
|
Added above. |
|
@bensie Are you able to look over this? |
|
This definitely seems problematic, though I'm not sure when I'll have time to dig in and take a look. |
|
I'm going through a similar problem but on the opposite direction. I'm fetching a large file and I want to be able to (according to @philister example) forward it to the request's response by chunks: tmp_path = Tempfile.create('animal-tmp') do |output|
while buffer = animal.picture.read(4096)
output.write(buffer)
end
output.path
end
send_file tmp_path, type: animal.picture.content_type, disposition: 'inline'And I noticed, that the PS.: I'm not using |
|
Sorry. Never mind. |
|
Is there any update on this? |
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this issue
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this issue
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this issue
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
DarkArc
added a commit
to topechelon/carrierwave
that referenced
this issue
Apr 12, 2018
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517. This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517. New behavior: In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object. In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents. The approach to storage requests is taken over two other approaches: Approach carrierwaveuploader#1: We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads. Approach carrierwaveuploader#2: We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
|
Fixed by #2314. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I believe the commit 927479c (PR #1517 and issue #1338) leads to a memory-problem when uploading large files:
I guess
file.readreads the complete file-content into memory. Fog can not "stream" the file from disk, using the multipart-feature correctly (when setfog_attributes[:multipart_chunk_size])At least, when I revert this commit, my large files (more GB than available mem) are uploaded. Current master returns a "NoMemoryError: failed to allocate memory".
The text was updated successfully, but these errors were encountered: