diff options
4 files changed, 90 insertions, 23 deletions
diff --git a/lib/bundler/vendor/compact_index_client/lib/compact_index_client/updater.rb b/lib/bundler/vendor/compact_index_client/lib/compact_index_client/updater.rb index f19b1df3d7..5c5ba41434 100644 --- a/lib/bundler/vendor/compact_index_client/lib/compact_index_client/updater.rb +++ b/lib/bundler/vendor/compact_index_client/lib/compact_index_client/updater.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "fileutils" require "stringio" +require "tmpdir" require "zlib" class Bundler::CompactIndexClient @@ -24,33 +26,41 @@ class Bundler::CompactIndexClient def update(local_path, remote_path, retrying = nil) headers = {} - if local_path.file? - headers["If-None-Match"] = etag_for(local_path) - headers["Range"] = "bytes=#{local_path.size}-" - else - # Fastly ignores Range when Accept-Encoding: gzip is set - headers["Accept-Encoding"] = "gzip" - end + Dir.mktmpdir(local_path.basename.to_s, local_path.dirname) do |local_temp_dir| + local_temp_path = Pathname.new(local_temp_dir).join(local_path.basename) - response = @fetcher.call(remote_path, headers) - return if response.is_a?(Net::HTTPNotModified) + # download new file if retrying + if retrying.nil? && local_path.file? + FileUtils.cp local_path, local_temp_path + headers["If-None-Match"] = etag_for(local_temp_path) + headers["Range"] = "bytes=#{local_temp_path.size}-" + else + # Fastly ignores Range when Accept-Encoding: gzip is set + headers["Accept-Encoding"] = "gzip" + end - content = response.body - if response["Content-Encoding"] == "gzip" - content = Zlib::GzipReader.new(StringIO.new(content)).read - end + response = @fetcher.call(remote_path, headers) + return if response.is_a?(Net::HTTPNotModified) + + content = response.body + if response["Content-Encoding"] == "gzip" + content = Zlib::GzipReader.new(StringIO.new(content)).read + end - mode = response.is_a?(Net::HTTPPartialContent) ? "a" : "w" - local_path.open(mode) {|f| f << content } + mode = response.is_a?(Net::HTTPPartialContent) ? "a" : "w" + local_temp_path.open(mode) {|f| f << content } - response_etag = response["ETag"] - return if etag_for(local_path) == response_etag + response_etag = response["ETag"] + if etag_for(local_temp_path) == response_etag + FileUtils.mv(local_temp_path, local_path) + return + end - if retrying.nil? - local_path.delete - update(local_path, remote_path, :retrying) - else - raise MisMatchedChecksumError.new(remote_path, response_etag, etag_for(local_path)) + if retrying.nil? + update(local_path, remote_path, :retrying) + else + raise MisMatchedChecksumError.new(remote_path, response_etag, etag_for(local_temp_path)) + end end end diff --git a/spec/install/gems/compact_index_spec.rb b/spec/install/gems/compact_index_spec.rb index ef3bc5aedc..e96936f01d 100644 --- a/spec/install/gems/compact_index_spec.rb +++ b/spec/install/gems/compact_index_spec.rb @@ -656,4 +656,22 @@ The checksum of /versions does not match the checksum provided by the server! So bundle! :install, :artifice => "compact_index_forbidden" end end + + it "performs partial update while local cache is updated by another process" do + gemfile <<-G + source "#{source_uri}" + gem 'rack' + G + + # Create an empty file to trigger a partial download + versions = File.join(Bundler.rubygems.user_home, ".bundle", "cache", "compact_index", + "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "versions") + FileUtils.mkdir_p(File.dirname(versions)) + FileUtils.touch(versions) + + bundle! :install, :artifice => "compact_index_concurrent_download" + + expect(File.read(versions)).to start_with("created_at") + should_be_installed "rack 1.0.0" + end end diff --git a/spec/support/artifice/compact_index.rb b/spec/support/artifice/compact_index.rb index 88706c3590..a9fe7112b0 100644 --- a/spec/support/artifice/compact_index.rb +++ b/spec/support/artifice/compact_index.rb @@ -40,7 +40,7 @@ class CompactIndexAPI < Endpoint if ranges status 206 - body ranges.map! {|range| response_body.byteslice(range) }.join + body ranges.map! {|range| slice_body(response_body, range) }.join else status 200 body response_body @@ -55,6 +55,14 @@ class CompactIndexAPI < Endpoint value ? value.split(/, ?/).select {|s| s.sub!(/"(.*)"/, '\1') } : [] end + def slice_body(body, range) + if body.respond_to?(:byteslice) + body.byteslice(range) + else # pre-1.9.3 + body.unpack("@#{range.first}a#{range.end + 1}").first + end + end + def gems(gem_repo = gem_repo1) @gems ||= {} @gems[gem_repo] ||= begin diff --git a/spec/support/artifice/compact_index_concurrent_download.rb b/spec/support/artifice/compact_index_concurrent_download.rb new file mode 100644 index 0000000000..30a2171a30 --- /dev/null +++ b/spec/support/artifice/compact_index_concurrent_download.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true +require File.expand_path("../compact_index", __FILE__) + +Artifice.deactivate + +class CompactIndexConcurrentDownload < CompactIndexAPI + get "/versions" do + versions = File.join(Bundler.rubygems.user_home, ".bundle", "cache", "compact_index", + "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "versions") + + # Verify the original (empty) content hasn't been deleted, e.g. on a retry + File.read(versions) == "" || raise("Original file should be present and empty") + + # Verify this is only requested once for a partial download + env["HTTP_RANGE"] || raise("Missing Range header for expected partial download") + + # Overwrite the file in parallel, which should be then overwritten + # after a successful download to prevent corruption + File.open(versions, "w") {|f| f.puts "another process" } + + etag_response do + file = tmp("versions.list") + file.delete if file.file? + file = CompactIndex::VersionsFile.new(file.to_s) + file.update_with(gems) + CompactIndex.versions(file, nil, {}) + end + end +end + +Artifice.activate_with(CompactIndexConcurrentDownload) |