Skip to content

Commit

Permalink
fix: dont retrieve from store until accessing file
Browse files Browse the repository at this point in the history
  • Loading branch information
Amnesthesia committed Sep 6, 2023
1 parent 8815592 commit 469402a
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 9 deletions.
6 changes: 5 additions & 1 deletion lib/carrierwave/mounter.rb
Expand Up @@ -66,7 +66,11 @@ def read_identifiers
def uploaders
@uploaders ||= read_identifiers.map do |identifier|
uploader = blank_uploader
uploader.retrieve_from_store!(identifier)

# Assign the identifier to the uploader here,
# instead of forcing the uploader to make a round-trip
# to the store if we already have the identifier.
uploader.identifier = identifier
uploader
end
end
Expand Down
2 changes: 0 additions & 2 deletions lib/carrierwave/uploader.rb
Expand Up @@ -42,8 +42,6 @@ module Uploader
# be trivial.
#
class Base
attr_reader :file

include CarrierWave::Uploader::Configuration
include CarrierWave::Uploader::Callbacks
include CarrierWave::Uploader::Proxy
Expand Down
2 changes: 1 addition & 1 deletion lib/carrierwave/uploader/configuration.rb
Expand Up @@ -214,7 +214,7 @@ def reset_config
config.base_path = CarrierWave.base_path
config.enable_processing = true
config.ensure_multipart_form = true
config.download_retry_count = 0
config.download_retry_count = 3
config.download_retry_wait_time = 5
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/carrierwave/uploader/mountable.rb
Expand Up @@ -3,6 +3,7 @@ module Uploader
module Mountable

attr_reader :model, :mounted_as
attr_writer :identifier

##
# If a model is given as the first parameter, it will be stored in the
Expand Down
6 changes: 5 additions & 1 deletion lib/carrierwave/uploader/remove.rb
Expand Up @@ -10,8 +10,12 @@ module Remove
#
def remove!
with_callbacks(:remove) do
@file.delete if @file
@file.delete if file
@file = nil
# Setting identifier to nil prevents
# file from attempting to retrieve_from_store!
# when accessed
@identifier = nil
@cache_id = nil
end
end
Expand Down
34 changes: 30 additions & 4 deletions lib/carrierwave/uploader/store.rb
Expand Up @@ -8,6 +8,8 @@ module Store
include CarrierWave::Uploader::Cache

included do
attr_accessor :retrieval_retry_count

prepend Module.new {
def initialize(*)
super
Expand Down Expand Up @@ -93,17 +95,41 @@ def store!(new_file=nil)
end
end

# Attempt to retrieve the file from the store
# if the file is nil, but to avoid calling this
# over and over if retrieving it returns nil,
# we keep track of the number of times we've
# attempted to retrieve it and just return nil
# if we exceed the maximum number of retries
#
# === Returns
#
# [CarrierWave::SanitizedFile, nil] the stored file or nil
def file
return @file unless @identifier
retrieve_from_store!(@identifier) unless @file
@file
end

##
# Retrieves the file from the storage.
#
# === Parameters
#
# [identifier (String)] uniquely identifies the file to retrieve
#
def retrieve_from_store!(identifier)
with_callbacks(:retrieve_from_store, identifier) do
@file = storage.retrieve!(identifier)
@identifier = identifier
def retrieve_from_store!(file_identifier)
self.retrieval_retry_count ||= 0
return if self.retrieval_retry_count > download_retry_count
self.retrieval_retry_count += 1
with_callbacks(:retrieve_from_store, file_identifier) do
# We can't retrieve the file if we have no identifier
# to retrieve it with. Identifier should be assigned
# when setting up the uploader, or when caching,
# or storing a new file
next unless file_identifier
@file = storage.retrieve!(file_identifier)
@identifier = file_identifier
end
end

Expand Down
1 change: 1 addition & 0 deletions spec/spec_helper.rb
Expand Up @@ -162,6 +162,7 @@ def stub_request(method, uri)
config.around :each, :with_retry do |example|
example.run_with_retry retry: 2
end
config.example_status_persistence_file_path = "tmp/failures.txt"
config.retry_callback = proc do |example|
sleep 1
end
Expand Down

0 comments on commit 469402a

Please sign in to comment.