Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: dont retrieve from store until accessing file #2698

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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