chore(storage): Error handling for open and unlinked files - #35031
chore(storage): Error handling for open and unlinked files#35031shubhangi-google wants to merge 5 commits into
Conversation
bc63dcf to
92e3408
Compare
| # Case 1: Input is a file path (String, Pathname, or object that responds to :to_path). | ||
| ::File.open Pathname(local_file).to_path, "rb" do |f| | ||
| digest_class.file(f).base64digest | ||
| if local_file.respond_to?(:read) |
There was a problem hiding this comment.
Why are we removing existing code?
There was a problem hiding this comment.
we are swapping the existing code to check for the :read capability (the IO-stream case) before falling back to the :to_path capability (the file path case).
There was a problem hiding this comment.
Still we're removing some of the cases. Can we try to keep our changes as minimum as possible?
There was a problem hiding this comment.
done kept relevant changes only
| # Case 1: Input is a file path (String, Pathname, or object that responds to :to_path). | ||
| ::File.open Pathname(local_file).to_path, "rb" do |f| | ||
| digest_class.file(f).base64digest | ||
| if local_file.respond_to?(:read) |
There was a problem hiding this comment.
Still we're removing some of the cases. Can we try to keep our changes as minimum as possible?
| if local_file.respond_to?(:read) | ||
| # Case 1: Input is an open stream (File or StringIO). | ||
| local_file.rewind if local_file.respond_to?(:rewind) | ||
| digest = digest_class.base64digest local_file.read |
There was a problem hiding this comment.
@shubhangi-google one thing to note about this solution:
digest = digest_class.base64digest local_file.read # <-- Entire file loaded into RAM!
The existing production flow does digest_class.file(f).base64digest which loads the file in chunks and uses O(1) memory. I'm wondering if it makes sense to do something like:
local_file.rewind if local_file.respond_to? :rewind
digest = digest_class.new
buf = ""
while local_file.read(16_384, buf)
digest.update buf
end
local_file.rewind if local_file.respond_to? :rewind
digest.base64digest
...
3 participants
Fixes issue where uploading unlinked temporary files (such as large payloads handled by the Puma web server) fails during the CRC32c digest computation.
closes #34987