From 9660aa4543deff41c60d131e081137f84e771499 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 8 Mar 2020 23:56:18 +0100 Subject: [PATCH] Change local media attachments to perform heavy processing asynchronously (#13210) Fix #9106 --- app/controllers/api/v1/media_controller.rb | 29 ++++++++++++---- app/controllers/api/v2/media_controller.rb | 12 +++++++ app/javascript/mastodon/actions/compose.js | 23 +++++++++++-- app/models/concerns/attachmentable.rb | 2 +- app/models/media_attachment.rb | 26 ++++++++++++-- .../rest/media_attachment_serializer.rb | 4 ++- app/services/post_status_service.rb | 1 + app/workers/backup_worker.rb | 8 +++-- app/workers/post_process_media_worker.rb | 34 +++++++++++++++++++ config/application.rb | 1 + config/locales/en.yml | 1 + config/routes.rb | 3 +- ...625_add_processing_to_media_attachments.rb | 5 +++ db/schema.rb | 3 +- lib/paperclip/attachment_extensions.rb | 30 ++++++++++++++++ 15 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 app/controllers/api/v2/media_controller.rb create mode 100644 app/workers/post_process_media_worker.rb create mode 100644 db/migrate/20200306035625_add_processing_to_media_attachments.rb create mode 100644 lib/paperclip/attachment_extensions.rb diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index d87d7b9466..0bb3d0d27b 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -3,25 +3,42 @@ class Api::V1::MediaController < Api::BaseController before_action -> { doorkeeper_authorize! :write, :'write:media' } before_action :require_user! + before_action :set_media_attachment, except: [:create] + before_action :check_processing, except: [:create] def create - @media = current_account.media_attachments.create!(media_params) - render json: @media, serializer: REST::MediaAttachmentSerializer + @media_attachment = current_account.media_attachments.create!(media_attachment_params) + render json: @media_attachment, serializer: REST::MediaAttachmentSerializer rescue Paperclip::Errors::NotIdentifiedByImageMagickError render json: file_type_error, status: 422 rescue Paperclip::Error render json: processing_error, status: 500 end + def show + render json: @media_attachment, serializer: REST::MediaAttachmentSerializer, status: status_code_for_media_attachment + end + def update - @media = current_account.media_attachments.where(status_id: nil).find(params[:id]) - @media.update!(media_params) - render json: @media, serializer: REST::MediaAttachmentSerializer + @media_attachment.update!(media_attachment_params) + render json: @media_attachment, serializer: REST::MediaAttachmentSerializer, status: status_code_for_media_attachment end private - def media_params + def status_code_for_media_attachment + @media_attachment.not_processed? ? 206 : 200 + end + + def set_media_attachment + @media_attachment = current_account.media_attachments.unattached.find(params[:id]) + end + + def check_processing + render json: processing_error, status: 422 if @media_attachment.processing_failed? + end + + def media_attachment_params params.permit(:file, :description, :focus) end diff --git a/app/controllers/api/v2/media_controller.rb b/app/controllers/api/v2/media_controller.rb new file mode 100644 index 0000000000..0c1baf01d0 --- /dev/null +++ b/app/controllers/api/v2/media_controller.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class Api::V2::MediaController < Api::V1::MediaController + def create + @media_attachment = current_account.media_attachments.create!({ delay_processing: true }.merge(media_attachment_params)) + render json: @media_attachment, serializer: REST::MediaAttachmentSerializer, status: 202 + rescue Paperclip::Errors::NotIdentifiedByImageMagickError + render json: file_type_error, status: 422 + rescue Paperclip::Error + render json: processing_error, status: 500 + end +end diff --git a/app/javascript/mastodon/actions/compose.js b/app/javascript/mastodon/actions/compose.js index c3c6ff1a1d..68ef8fbd54 100644 --- a/app/javascript/mastodon/actions/compose.js +++ b/app/javascript/mastodon/actions/compose.js @@ -230,12 +230,31 @@ export function uploadCompose(files) { // Account for disparity in size of original image and resized data total += file.size - f.size; - return api(getState).post('/api/v1/media', data, { + return api(getState).post('/api/v2/media', data, { onUploadProgress: function({ loaded }){ progress[i] = loaded; dispatch(uploadComposeProgress(progress.reduce((a, v) => a + v, 0), total)); }, - }).then(({ data }) => dispatch(uploadComposeSuccess(data, f))); + }).then(({ status, data }) => { + // If server-side processing of the media attachment has not completed yet, + // poll the server until it is, before showing the media attachment as uploaded + + if (status === 200) { + dispatch(uploadComposeSuccess(data, f)); + } else if (status === 202) { + const poll = () => { + api(getState).get(`/api/v1/media/${data.id}`).then(response => { + if (response.status === 200) { + dispatch(uploadComposeSuccess(data, f)); + } else if (response.status === 206) { + setTimeout(() => poll(), 1000); + } + }).catch(error => dispatch(uploadComposeFail(error))); + }; + + poll(); + } + }); }).catch(error => dispatch(uploadComposeFail(error))); }; }; diff --git a/app/models/concerns/attachmentable.rb b/app/models/concerns/attachmentable.rb index 43ff8ac12e..18b872c1e6 100644 --- a/app/models/concerns/attachmentable.rb +++ b/app/models/concerns/attachmentable.rb @@ -74,7 +74,7 @@ module Attachmentable self.class.attachment_definitions.each_key do |attachment_name| attachment = send(attachment_name) - next if attachment.blank? || attachment.queued_for_write[:original].blank? + next if attachment.blank? || attachment.queued_for_write[:original].blank? || attachment.options[:preserve_files] attachment.instance_write :file_name, SecureRandom.hex(8) + File.extname(attachment.instance_read(:file_name)) end diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 1e36625ca9..3fd4f231c1 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -19,12 +19,14 @@ # description :text # scheduled_status_id :bigint(8) # blurhash :string +# processing :integer # class MediaAttachment < ApplicationRecord self.inheritance_column = nil enum type: [:image, :gifv, :video, :unknown, :audio] + enum processing: [:queued, :in_progress, :complete, :failed], _prefix: true MAX_DESCRIPTION_LENGTH = 1_500 @@ -156,6 +158,10 @@ class MediaAttachment < ApplicationRecord remote_url.blank? end + def not_processed? + processing.present? && !processing_complete? + end + def needs_redownload? file.blank? && remote_url.present? end @@ -203,10 +209,18 @@ class MediaAttachment < ApplicationRecord "#{x},#{y}" end + attr_writer :delay_processing + + def delay_processing? + @delay_processing + end + + after_commit :enqueue_processing, on: :create after_commit :reset_parent_cache, on: :update before_create :prepare_description, unless: :local? before_create :set_shortcode + before_create :set_processing before_post_process :set_type_and_extension @@ -277,6 +291,10 @@ class MediaAttachment < ApplicationRecord end end + def set_processing + self.processing = delay_processing? ? :queued : :complete + end + def set_meta meta = populate_meta @@ -322,9 +340,11 @@ class MediaAttachment < ApplicationRecord }.compact end - def reset_parent_cache - return if status_id.nil? + def enqueue_processing + PostProcessMediaWorker.perform_async(id) if delay_processing? + end - Rails.cache.delete("statuses/#{status_id}") + def reset_parent_cache + Rails.cache.delete("statuses/#{status_id}") if status_id.present? end end diff --git a/app/serializers/rest/media_attachment_serializer.rb b/app/serializers/rest/media_attachment_serializer.rb index 1b3498ea4e..cc10e30015 100644 --- a/app/serializers/rest/media_attachment_serializer.rb +++ b/app/serializers/rest/media_attachment_serializer.rb @@ -12,7 +12,9 @@ class REST::MediaAttachmentSerializer < ActiveModel::Serializer end def url - if object.needs_redownload? + if object.not_processed? + nil + elsif object.needs_redownload? media_proxy_url(object.id, :original) else full_asset_url(object.file.url(:original)) diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 2301621ab8..c61b3baa25 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -101,6 +101,7 @@ class PostStatusService < BaseService @media = @account.media_attachments.where(status_id: nil).where(id: @options[:media_ids].take(4).map(&:to_i)) raise Mastodon::ValidationError, I18n.t('media_attachments.validations.images_and_video') if @media.size > 1 && @media.find(&:audio_or_video?) + raise Mastodon::ValidationError, I18n.t('media_attachments.validations.not_ready') if @media.any?(&:not_processed?) end def language_from_option(str) diff --git a/app/workers/backup_worker.rb b/app/workers/backup_worker.rb index e4c609d706..7b0b52844d 100644 --- a/app/workers/backup_worker.rb +++ b/app/workers/backup_worker.rb @@ -9,8 +9,12 @@ class BackupWorker backup_id = msg['args'].first ActiveRecord::Base.connection_pool.with_connection do - backup = Backup.find(backup_id) - backup&.destroy + begin + backup = Backup.find(backup_id) + backup.destroy + rescue ActiveRecord::RecordNotFound + true + end end end diff --git a/app/workers/post_process_media_worker.rb b/app/workers/post_process_media_worker.rb new file mode 100644 index 0000000000..d3ebda194a --- /dev/null +++ b/app/workers/post_process_media_worker.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class PostProcessMediaWorker + include Sidekiq::Worker + + sidekiq_options retry: 1, dead: false + + sidekiq_retries_exhausted do |msg| + media_attachment_id = msg['args'].first + + ActiveRecord::Base.connection_pool.with_connection do + begin + media_attachment = MediaAttachment.find(media_attachment_id) + media_attachment.processing = :failed + media_attachment.save + rescue ActiveRecord::RecordNotFound + true + end + end + + Sidekiq.logger.error("Processing media attachment #{media_attachment_id} failed with #{msg['error_message']}") + end + + def perform(media_attachment_id) + media_attachment = MediaAttachment.find(media_attachment_id) + media_attachment.processing = :in_progress + media_attachment.save + media_attachment.file.reprocess_original! + media_attachment.processing = :complete + media_attachment.save + rescue ActiveRecord::RecordNotFound + true + end +end diff --git a/config/application.rb b/config/application.rb index 1baa166ce6..cd599eefce 100644 --- a/config/application.rb +++ b/config/application.rb @@ -7,6 +7,7 @@ require 'rails/all' Bundler.require(*Rails.groups) require_relative '../app/lib/exceptions' +require_relative '../lib/paperclip/attachment_extensions' require_relative '../lib/paperclip/lazy_thumbnail' require_relative '../lib/paperclip/gif_transcoder' require_relative '../lib/paperclip/video_transcoder' diff --git a/config/locales/en.yml b/config/locales/en.yml index 8440b471cd..a031f9e9da 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -853,6 +853,7 @@ en: media_attachments: validations: images_and_video: Cannot attach a video to a status that already contains images + not_ready: Cannot attach files that have not finished processing. Try again in a moment! too_many: Cannot attach more than 4 files migrations: acct: Moved to diff --git a/config/routes.rb b/config/routes.rb index 2de31e2dba..89d446d10f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -343,7 +343,7 @@ Rails.application.routes.draw do end end - resources :media, only: [:create, :update] + resources :media, only: [:create, :update, :show] resources :blocks, only: [:index] resources :mutes, only: [:index] resources :favourites, only: [:index] @@ -455,6 +455,7 @@ Rails.application.routes.draw do end namespace :v2 do + resources :media, only: [:create] get '/search', to: 'search#index', as: :search end diff --git a/db/migrate/20200306035625_add_processing_to_media_attachments.rb b/db/migrate/20200306035625_add_processing_to_media_attachments.rb new file mode 100644 index 0000000000..131ffa52ae --- /dev/null +++ b/db/migrate/20200306035625_add_processing_to_media_attachments.rb @@ -0,0 +1,5 @@ +class AddProcessingToMediaAttachments < ActiveRecord::Migration[5.2] + def change + add_column :media_attachments, :processing, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index b09ee0e763..ddb9a5d8c8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_01_26_203551) do +ActiveRecord::Schema.define(version: 2020_03_06_035625) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -459,6 +459,7 @@ ActiveRecord::Schema.define(version: 2020_01_26_203551) do t.text "description" t.bigint "scheduled_status_id" t.string "blurhash" + t.integer "processing" t.index ["account_id"], name: "index_media_attachments_on_account_id" t.index ["scheduled_status_id"], name: "index_media_attachments_on_scheduled_status_id" t.index ["shortcode"], name: "index_media_attachments_on_shortcode", unique: true diff --git a/lib/paperclip/attachment_extensions.rb b/lib/paperclip/attachment_extensions.rb new file mode 100644 index 0000000000..3b308af5f9 --- /dev/null +++ b/lib/paperclip/attachment_extensions.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Paperclip + module AttachmentExtensions + # We overwrite this method to support delayed processing in + # Sidekiq. Since we process the original file to reduce disk + # usage, and we still want to generate thumbnails straight + # away, it's the only style we need to exclude + def process_style?(style_name, style_args) + if style_name == :original && instance.respond_to?(:delay_processing?) && instance.delay_processing? + false + else + style_args.empty? || style_args.include?(style_name) + end + end + + def reprocess_original! + old_original_path = path(:original) + reprocess!(:original) + new_original_path = path(:original) + + if new_original_path != old_original_path + @queued_for_delete << old_original_path + flush_deletes + end + end + end +end + +Paperclip::Attachment.prepend(Paperclip::AttachmentExtensions)