From 9cdfbba6c2b1cb6d08ff126f44f5f0df48affc6e Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 18 Sep 2018 10:01:03 -0500 Subject: [PATCH] Fix #3910: Corrupted images during upload. Use a fresh tempfile for each download attempt instead of reusing the same file (and having to rewind/truncate it after each failed attempt). --- app/logical/downloads/file.rb | 10 ++++------ test/unit/downloads/file_test.rb | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/app/logical/downloads/file.rb b/app/logical/downloads/file.rb index eb2aece9c..bbd54b01e 100644 --- a/app/logical/downloads/file.rb +++ b/app/logical/downloads/file.rb @@ -54,17 +54,15 @@ module Downloads end end - def download!(tries: 3) + def download!(tries: 3, **options) strategy = Sources::Strategies.find(source, referer) - output_file = Tempfile.new(binmode: true) url = self.class.uncached_url(strategy.file_url, strategy.headers) @data = strategy.data Retriable.retriable(on: RETRIABLE_ERRORS, tries: tries, base_interval: 0) do - http_get_streaming(url, output_file, strategy.headers) + file = http_get_streaming(url, headers: strategy.headers, **options) + return [file, strategy] end - - [output_file, strategy] end def validate_local_hosts(url) @@ -74,7 +72,7 @@ module Downloads end end - def http_get_streaming(src, file, headers = {}, max_size: Danbooru.config.max_file_size) + def http_get_streaming(src, file: Tempfile.new(binmode: true), headers: {}, max_size: Danbooru.config.max_file_size) url = URI.parse(src) unless url.is_a?(URI::HTTP) || url.is_a?(URI::HTTPS) diff --git a/test/unit/downloads/file_test.rb b/test/unit/downloads/file_test.rb index 435acacd9..d742044c5 100644 --- a/test/unit/downloads/file_test.rb +++ b/test/unit/downloads/file_test.rb @@ -6,7 +6,6 @@ module Downloads setup do @source = "http://www.google.com/intl/en_ALL/images/logo.gif" @download = Downloads::File.new(@source) - @tempfile = Tempfile.new("danbooru-test") end context "that fails" do @@ -14,11 +13,22 @@ module Downloads HTTParty.expects(:get).times(3).raises(Errno::ETIMEDOUT) assert_raises(Errno::ETIMEDOUT) { @download.download! } end + + should "return an uncorrupted file on the second try" do + bomb = stub("bomb") + bomb.expects(:size).raises(IOError) + resp = stub("resp", success?: true) + + HTTParty.expects(:get).twice.multiple_yields("a", bomb, "b", "c").then.multiple_yields("a", "b", "c").returns(resp) + tempfile, _ = @download.download! + + assert_equal("abc", tempfile.read) + end end should "throw an exception when the file is larger than the maximum" do assert_raise(Downloads::File::Error) do - @download.http_get_streaming(@source, @tempfile, {}, max_size: 1) + @download.download!(max_size: 1) end end