Skip to content

Commit 5d970e0

Browse files
author
Glyn Normington
committed
Merge 57311364-retry-failing-downloads to master
[Completes #57311364]
2 parents 3cb88f0 + c022a49 commit 5d970e0

2 files changed

Lines changed: 77 additions & 20 deletions

File tree

‎lib/java_buildpack/util/download_cache.rb‎

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ module JavaBuildpack::Util
2929
# A cache for downloaded files that is configured to use a filesystem as the backing store. This cache uses standard
3030
# file locking (<tt>File.flock()</tt>) in order ensure that mutation of files in the cache is non-concurrent across
3131
# processes. Reading files (once they've been downloaded) happens concurrently so read performance is not impacted.
32-
class DownloadCache
32+
class DownloadCache # rubocop:disable ClassLength
3333

3434
# Creates an instance of the cache that is backed by the filesystem rooted at +cache_root+
3535
#
@@ -127,6 +127,12 @@ def evict(uri)
127127

128128
HTTP_OK = '200'.freeze
129129

130+
TIMEOUT_SECONDS = 10
131+
132+
INTERNET_DETECTION_RETRY_LIMIT = 5
133+
134+
DOWNLOAD_RETRY_LIMIT = 5
135+
130136
@@monitor = Monitor.new
131137
@@internet_checked = false
132138
@@internet_up = true
@@ -136,8 +142,6 @@ def self.get_configuration
136142
YAML.load_file(expanded_path)
137143
end
138144

139-
TIMEOUT_SECONDS = 10
140-
141145
def self.internet_available?(filenames, uri, logger)
142146
@@monitor.synchronize do
143147
return @@internet_up, false if @@internet_checked # rubocop:disable RedundantReturn
@@ -147,16 +151,12 @@ def self.internet_available?(filenames, uri, logger)
147151
return store_internet_availability(false), false # rubocop:disable RedundantReturn
148152
elsif cache_configuration['remote_downloads'] == 'enabled'
149153
begin
150-
rich_uri = URI(uri)
151-
152154
# Beware known problems with timeouts: https://www.ruby-forum.com/topic/143840
153-
Net::HTTP.start(rich_uri.host, rich_uri.port, use_ssl: DownloadCache.use_ssl?(rich_uri), read_timeout: TIMEOUT_SECONDS, connect_timeout: TIMEOUT_SECONDS, open_timeout: TIMEOUT_SECONDS) do |http|
154-
request = Net::HTTP::Get.new(uri)
155-
http.request request do |response|
156-
internet_up = response.code == HTTP_OK
157-
write_response(filenames, response) if internet_up
158-
return store_internet_availability(internet_up), internet_up # rubocop:disable RedundantReturn
159-
end
155+
opts = { read_timeout: TIMEOUT_SECONDS, connect_timeout: TIMEOUT_SECONDS, open_timeout: TIMEOUT_SECONDS }
156+
return http_get(uri, INTERNET_DETECTION_RETRY_LIMIT, logger, opts) do |response|
157+
internet_up = response.code == HTTP_OK
158+
write_response(filenames, response) if internet_up
159+
return store_internet_availability(internet_up), internet_up # rubocop:disable RedundantReturn
160160
end
161161
rescue *HTTP_ERRORS => ex
162162
logger.debug { "Internet detection failed with #{ex}" }
@@ -167,6 +167,26 @@ def self.internet_available?(filenames, uri, logger)
167167
end
168168
end
169169

170+
def self.http_get(uri, retry_limit, logger, opts = {})
171+
rich_uri = URI(uri)
172+
options = opts.merge(use_ssl: DownloadCache.use_ssl?(rich_uri))
173+
Net::HTTP.start(rich_uri.host, rich_uri.port, options) do |http|
174+
request = Net::HTTP::Get.new(uri)
175+
1.upto(retry_limit) do |try|
176+
begin
177+
http.request request do |response|
178+
if response.code == HTTP_OK || try == retry_limit
179+
return yield response
180+
end
181+
end
182+
rescue *HTTP_ERRORS => ex
183+
logger.debug { "HTTP get attempt #{try} of #{retry_limit} failed: #{ex}" }
184+
raise ex if try == retry_limit
185+
end
186+
end
187+
end
188+
end
189+
170190
def self.store_internet_availability(internet_up)
171191
@@monitor.synchronize do
172192
@@internet_up = internet_up
@@ -188,15 +208,9 @@ def delete_file(filename)
188208
def download(filenames, uri, internet_up)
189209
if internet_up
190210
begin
191-
rich_uri = URI(uri)
192-
193-
Net::HTTP.start(rich_uri.host, rich_uri.port, use_ssl: DownloadCache.use_ssl?(rich_uri)) do |http|
194-
request = Net::HTTP::Get.new(uri)
195-
http.request request do |response|
196-
DownloadCache.write_response(filenames, response)
197-
end
211+
DownloadCache.http_get(uri, DOWNLOAD_RETRY_LIMIT, @logger) do |response|
212+
DownloadCache.write_response(filenames, response)
198213
end
199-
200214
rescue *HTTP_ERRORS => ex
201215
puts 'FAIL'
202216
error_message = "Unable to download from #{uri} due to #{ex}"

‎spec/java_buildpack/util/download_cache_spec.rb‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,25 @@ def suppress_internet_availability_check
9494
end
9595
end
9696

97+
it 'should not raise error if download cannot be completed but retrying succeeds' do
98+
stub_request(:get, 'http://foo-uri/').to_raise(SocketError).then.to_return(
99+
status: 200,
100+
body: 'foo-cached',
101+
headers: {
102+
Etag: 'foo-etag',
103+
'Last-Modified' => 'foo-last-modified'
104+
}
105+
)
106+
107+
Dir.mktmpdir do |root|
108+
DownloadCache.new(root).get('http://foo-uri/') {}
109+
110+
expect_file_content root, 'cached', 'foo-cached'
111+
expect_file_content root, 'etag', 'foo-etag'
112+
expect_file_content root, 'last_modified', 'foo-last-modified'
113+
end
114+
end
115+
97116
it 'should download from a uri if the cached file exists and etag exists' do
98117
suppress_internet_availability_check
99118

@@ -355,6 +374,30 @@ def suppress_internet_availability_check
355374
end
356375
end
357376

377+
it 'should not use the buildpack cache if the download cannot be completed but a retry succeeds' do
378+
stub_request(:get, 'http://foo-uri/').to_raise(SocketError).then.to_return(
379+
status: 200,
380+
body: 'foo-cached',
381+
headers: {
382+
Etag: 'foo-etag',
383+
'Last-Modified' => 'foo-last-modified'
384+
}
385+
)
386+
387+
Dir.mktmpdir do |root|
388+
Dir.mktmpdir do |buildpack_cache|
389+
java_buildpack_cache = File.join(buildpack_cache, 'java-buildpack')
390+
FileUtils.mkdir_p java_buildpack_cache
391+
touch java_buildpack_cache, 'cached', 'foo-stashed'
392+
with_buildpack_cache(buildpack_cache) do
393+
DownloadCache.new(root).get('http://foo-uri/') do |file|
394+
expect(file.read).to eq('foo-cached')
395+
end
396+
end
397+
end
398+
end
399+
end
400+
358401
it 'should use the buildpack cache if the download cannot be completed because Errno::ENETUNREACH is raised' do
359402
stub_request(:get, 'http://foo-uri/').to_raise(Errno::ENETUNREACH)
360403

0 commit comments

Comments
 (0)