Merge pull request #3700 from evazion/fix-3659

Fix uploads getting stuck in 'processing' state (fix #3659)

app/logical/downloads/file.rb

@@ -94,7 +94,7 @@ module Downloads
           else
             raise Error.new("HTTP error code: #{res.code} #{res.message}")
           end
-        rescue Errno::ECONNRESET, Errno::ETIMEDOUT, Errno::EIO, Errno::EHOSTUNREACH, Errno::ECONNREFUSED, IOError => x
+        rescue Errno::ECONNRESET, Errno::ETIMEDOUT, Errno::EIO, Errno::EHOSTUNREACH, Errno::ECONNREFUSED, Timeout::Error, IOError => x
           tries += 1
           if tries < 3
             retry

app/models/upload.rb

@@ -165,25 +165,11 @@ class Upload < ApplicationRecord
     end
 
     def process!(force = false)
-      @tries ||= 0
-      return if !force && status =~ /processing|completed|error/
-
       process_upload
       post = create_post_from_upload
-
-    rescue Timeout::Error, Net::HTTP::Persistent::Error => x
-      if @tries > 3
-        update_attributes(:status => "error: #{x.class} - #{x.message}", :backtrace => x.backtrace.join("\n"))
-      else
-        @tries += 1
-        retry
-      end
-      nil
-
     rescue Exception => x
       update_attributes(:status => "error: #{x.class} - #{x.message}", :backtrace => x.backtrace.join("\n"))
       nil
-
     ensure
       file.try(:close!)
     end

test/unit/upload_test.rb

@@ -57,30 +57,17 @@ class UploadTest < ActiveSupport::TestCase
         should "discover the dimensions for a GIF" do
           @upload = FactoryBot.create(:upload, file: upload_file("test/files/test.gif"))
           assert_equal([400, 400], @upload.calculate_dimensions)
-          @upload = FactoryBot.create(:upload, :file_path => "#{Rails.root}/test/files/compressed.swf")
-          @upload.calculate_dimensions
-          assert_equal(607, @upload.image_width)
-          assert_equal(756, @upload.image_height)
         end
       end
 
       context "content type calculator" do
         should "know how to parse jpeg, png, gif, and swf file headers" do
           @upload = FactoryBot.create(:jpg_upload)
-          assert_equal("image/jpeg", @upload.file_header_to_content_type("#{Rails.root}/test/files/test.jpg"))
+          assert_equal("jpg", @upload.file_header_to_file_ext(File.open("#{Rails.root}/test/files/test.jpg")))
-          assert_equal("image/gif", @upload.file_header_to_content_type("#{Rails.root}/test/files/test.gif"))
+          assert_equal("gif", @upload.file_header_to_file_ext(File.open("#{Rails.root}/test/files/test.gif")))
-          assert_equal("image/png", @upload.file_header_to_content_type("#{Rails.root}/test/files/test.png"))
+          assert_equal("png", @upload.file_header_to_file_ext(File.open("#{Rails.root}/test/files/test.png")))
-          assert_equal("application/x-shockwave-flash", @upload.file_header_to_content_type("#{Rails.root}/test/files/compressed.swf"))
+          assert_equal("swf", @upload.file_header_to_file_ext(File.open("#{Rails.root}/test/files/compressed.swf")))
-          assert_equal("application/octet-stream", @upload.file_header_to_content_type("#{Rails.root}/README.md"))
+          assert_equal("bin", @upload.file_header_to_file_ext(File.open("#{Rails.root}/README.md")))
-        end
-
-        should "know how to parse jpeg, png, gif, and swf content types" do
-          @upload = FactoryBot.create(:jpg_upload)
-          assert_equal("jpg", @upload.content_type_to_file_ext("image/jpeg"))
-          assert_equal("gif", @upload.content_type_to_file_ext("image/gif"))
-          assert_equal("png", @upload.content_type_to_file_ext("image/png"))
-          assert_equal("swf", @upload.content_type_to_file_ext("application/x-shockwave-flash"))
-          assert_equal("bin", @upload.content_type_to_file_ext(""))
         end
       end
 
@@ -97,7 +84,6 @@ class UploadTest < ActiveSupport::TestCase
           setup do
             @url = "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=46378654"
             @upload = FactoryBot.create(:source_upload, :source => @url, :tag_string => "ugoira")
-            @output_file = Tempfile.new("download")
           end
 
           should "process successfully" do
@@ -106,15 +92,6 @@ class UploadTest < ActiveSupport::TestCase
             assert_equal("zip", @upload.file_header_to_file_ext(output_file))
           end
         end
-
-        should "initialize the final path after downloading a file" do
-          @upload = FactoryBot.create(:source_upload)
-          path = "#{Rails.root}/tmp/test.download.jpg"
-          assert_nothing_raised {@upload.download_from_source(path)}
-          assert(File.exists?(path))
-          assert_equal(8558, File.size(path))
-          assert_equal(path, @upload.file_path)
-        end
       end
 
       context "determining if a file is downloadable" do
@@ -151,21 +128,17 @@ class UploadTest < ActiveSupport::TestCase
       context "hash calculator" do
         should "caculate the hash" do
           @upload = FactoryBot.create(:jpg_upload)
-          @upload.calculate_hash(@upload.file_path)
+          @upload.process_upload
           assert_equal("ecef68c44edb8a0d6a3070b5f8e8ee76", @upload.md5)
         end
       end
 
       context "resizer" do
         should "generate several resized versions of the image" do
-          @upload = FactoryBot.create(:large_jpg_upload)
+          @upload = FactoryBot.create(:upload, file_ext: "jpg", image_width: 1356, image_height: 911, file: upload_file("test/files/test-large.jpg"))
-          @upload.calculate_hash(@upload.file_path)
+          preview_file, sample_file = @upload.generate_resizes
-          @upload.calculate_dimensions(@upload.file_path)
+          assert_operator(preview_file.size, :>, 1_000)
-          assert_nothing_raised {@upload.generate_resizes(@upload.file_path)}
+          assert_operator(sample_file.size, :>, 1_000)
-          assert(File.exists?(@upload.resized_file_path_for(Danbooru.config.small_image_width)))
-          assert(File.size(@upload.resized_file_path_for(Danbooru.config.small_image_width)) > 0)
-          assert(File.exists?(@upload.resized_file_path_for(Danbooru.config.large_image_width)))
-          assert(File.size(@upload.resized_file_path_for(Danbooru.config.large_image_width)) > 0)
         end
       end
 
@@ -180,13 +153,10 @@ class UploadTest < ActiveSupport::TestCase
       context "with an artist commentary" do
         setup do
           @upload = FactoryBot.create(:source_upload,
-            :rating => "s",
+            include_artist_commentary: "1",
-            :uploader_ip_addr => "127.0.0.1",
+            artist_commentary_title: "",
-            :tag_string => "hoge foo"
+            artist_commentary_desc: "blah",
-            )
+          )
-          @upload.include_artist_commentary = "1"
-          @upload.artist_commentary_title = ""
-          @upload.artist_commentary_desc = "blah"
         end
 
         should "create an artist commentary when processed" do
@@ -250,13 +220,8 @@ class UploadTest < ActiveSupport::TestCase
 
     should "process completely for a pixiv ugoira" do
       skip "ffmpeg is not installed" unless check_ffmpeg
-      
+      @upload = FactoryBot.create(:source_upload, source: "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=46378654")
-      @upload = FactoryBot.create(:source_upload,
+
-        :source => "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=46378654",
-        :rating => "s",
-        :uploader_ip_addr => "127.0.0.1",
-        :tag_string => "hoge foo"
-        )
       assert_difference(["PixivUgoiraFrameData.count", "Post.count"]) do
         @upload.process!
         assert_equal([], @upload.errors.full_messages)
@@ -268,7 +233,6 @@ class UploadTest < ActiveSupport::TestCase
       assert_equal(60, post.image_height)
       assert_equal("https://i.pximg.net/img-zip-ugoira/img/2014/10/05/23/42/23/46378654_ugoira1920x1080.zip", post.source)
       assert_nothing_raised { post.file(:original) }
-      assert_nothing_raised { post.file(:large) }
       assert_nothing_raised { post.file(:preview) }
     end
 
@@ -342,15 +306,14 @@ class UploadTest < ActiveSupport::TestCase
       end
     end
 
-    should "delete the temporary file upon completion" do
+    context "on timeout errors" do
-      @upload = FactoryBot.create(:source_upload,
+      should "leave the upload in an error state" do
-        :rating => "s",
+        HTTParty.stubs(:get).raises(Net::ReadTimeout)
-        :uploader_ip_addr => "127.0.0.1",
+        @upload = FactoryBot.create(:source_upload)
-        :tag_string => "hoge foo"
+        @upload.process!
-      )
 
-      @upload.process!
+        assert_match(/\Aerror/, @upload.status)
-      assert(!File.exists?(@upload.temp_file_path))
+      end
     end
   end
 end