Fix #1780 (Fog based file upload streaming) #2303
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this is about
This fixes an issue with file upload streaming. Previously passing the file was added in PR #468, and was subsequently broken by PR #1517.
This corrects the approach implemented in PR #468, without introducing the problem in PR #1517.
Approaches
I came up with three different approach for file upload:
Approach A
We could read the file into memory as carrierwave currently does. This results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads.
Approach B
We could set a flag to mark a stream uploaded file as needing retrieved. This would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for users making use of the current code.
Approach C
We could store a reference to the source file, and then use that for reading. This would result in issues reading the uploaded file content if the source file is a temp file that goes out of scope.
Chosen approach
I ended up using Approach B, as I think it's the least buggy/best for most people. However, I'm open to feedback, my first commit in this PR is Approach C, however, the second commit changes it back to Approach B.
I wanted to open up a pull request to see what the general carrierwave community thought was the best approach to fix streaming support.