From f1c1dd0118c5d222c5bf48ed8d223fe39ecde39e Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Tue, 2 May 2023 12:16:07 -0400 Subject: [PATCH] Rename `with_lock` to `with_redis_lock` to avoid confusion with ActiveRecord's method (#24741) --- .rubocop_todo.yml | 7 ------- app/controllers/media_proxy_controller.rb | 2 +- app/controllers/settings/exports_controller.rb | 2 +- app/lib/activitypub/activity/announce.rb | 2 +- app/lib/activitypub/activity/create.rb | 4 ++-- app/lib/activitypub/activity/delete.rb | 6 +++--- app/models/account_migration.rb | 2 +- app/models/concerns/lockable.rb | 2 +- app/services/activitypub/process_account_service.rb | 2 +- app/services/activitypub/process_status_update_service.rb | 4 ++-- app/services/fetch_link_card_service.rb | 2 +- app/services/remove_status_service.rb | 2 +- app/services/resolve_account_service.rb | 2 +- app/services/unfollow_service.rb | 2 +- app/services/vote_service.rb | 2 +- app/workers/distribution_worker.rb | 2 +- 16 files changed, 19 insertions(+), 26 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 17b0a33a0a..5e2e908927 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1410,13 +1410,6 @@ Rails/SkipsModelValidations: - 'spec/services/follow_service_spec.rb' - 'spec/services/update_account_service_spec.rb' -Rails/TransactionExitStatement: - Exclude: - - 'app/lib/activitypub/activity/announce.rb' - - 'app/lib/activitypub/activity/create.rb' - - 'app/lib/activitypub/activity/delete.rb' - - 'app/services/activitypub/process_account_service.rb' - # Configuration parameters: Include. # Include: app/models/**/*.rb Rails/UniqueValidationWithoutIndex: diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb index 1b5486c122..8d480d704e 100644 --- a/app/controllers/media_proxy_controller.rb +++ b/app/controllers/media_proxy_controller.rb @@ -16,7 +16,7 @@ class MediaProxyController < ApplicationController rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error def show - with_lock("media_download:#{params[:id]}") do + with_redis_lock("media_download:#{params[:id]}") do @media_attachment = MediaAttachment.remote.attached.find(params[:id]) authorize @media_attachment.status, :show? redownload! if @media_attachment.needs_redownload? && !reject_media? diff --git a/app/controllers/settings/exports_controller.rb b/app/controllers/settings/exports_controller.rb index deaa7940eb..46a340aeb3 100644 --- a/app/controllers/settings/exports_controller.rb +++ b/app/controllers/settings/exports_controller.rb @@ -15,7 +15,7 @@ class Settings::ExportsController < Settings::BaseController def create backup = nil - with_lock("backup:#{current_user.id}") do + with_redis_lock("backup:#{current_user.id}") do authorize :backup, :create? backup = current_user.backups.create! end diff --git a/app/lib/activitypub/activity/announce.rb b/app/lib/activitypub/activity/announce.rb index e6674be8ae..9dcafff3ab 100644 --- a/app/lib/activitypub/activity/announce.rb +++ b/app/lib/activitypub/activity/announce.rb @@ -4,7 +4,7 @@ class ActivityPub::Activity::Announce < ActivityPub::Activity def perform return reject_payload! if delete_arrived_first?(@json['id']) || !related_to_local_activity? - with_lock("announce:#{value_or_id(@object)}") do + with_redis_lock("announce:#{value_or_id(@object)}") do original_status = status_from_object return reject_payload! if original_status.nil? || !announceable?(original_status) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 23fbabf4cc..28cea8ec04 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -47,7 +47,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def create_status return reject_payload! if unsupported_object_type? || non_matching_uri_hosts?(@account.uri, object_uri) || tombstone_exists? || !related_to_local_activity? - with_lock("create:#{object_uri}") do + with_redis_lock("create:#{object_uri}") do return if delete_arrived_first?(object_uri) || poll_vote? @status = find_existing_status @@ -313,7 +313,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity poll = replied_to_status.preloadable_poll already_voted = true - with_lock("vote:#{replied_to_status.poll_id}:#{@account.id}") do + with_redis_lock("vote:#{replied_to_status.poll_id}:#{@account.id}") do already_voted = poll.votes.where(account: @account).exists? poll.votes.create!(account: @account, choice: poll.options.index(@object['name']), uri: object_uri) end diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb index 3af500f2b1..61f6ca6997 100644 --- a/app/lib/activitypub/activity/delete.rb +++ b/app/lib/activitypub/activity/delete.rb @@ -12,7 +12,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity private def delete_person - with_lock("delete_in_progress:#{@account.id}", autorelease: 2.hours, raise_on_failure: false) do + with_redis_lock("delete_in_progress:#{@account.id}", autorelease: 2.hours, raise_on_failure: false) do DeleteAccountService.new.call(@account, reserve_username: false, skip_activitypub: true) end end @@ -20,14 +20,14 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity def delete_note return if object_uri.nil? - with_lock("delete_status_in_progress:#{object_uri}", raise_on_failure: false) do + with_redis_lock("delete_status_in_progress:#{object_uri}", raise_on_failure: false) do unless non_matching_uri_hosts?(@account.uri, object_uri) # This lock ensures a concurrent `ActivityPub::Activity::Create` either # does not create a status at all, or has finished saving it to the # database before we try to load it. # Without the lock, `delete_later!` could be called after `delete_arrived_first?` # and `Status.find` before `Status.create!` - with_lock("create:#{object_uri}") { delete_later!(object_uri) } + with_redis_lock("create:#{object_uri}") { delete_later!(object_uri) } Tombstone.find_or_create_by(uri: object_uri, account: @account) end diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb index fa8cb6013c..b9da596172 100644 --- a/app/models/account_migration.rb +++ b/app/models/account_migration.rb @@ -42,7 +42,7 @@ class AccountMigration < ApplicationRecord return false unless errors.empty? - with_lock("account_migration:#{account.id}") do + with_redis_lock("account_migration:#{account.id}") do save end end diff --git a/app/models/concerns/lockable.rb b/app/models/concerns/lockable.rb index 55a9714ca8..3354ce1a99 100644 --- a/app/models/concerns/lockable.rb +++ b/app/models/concerns/lockable.rb @@ -5,7 +5,7 @@ module Lockable # @param [ActiveSupport::Duration] autorelease Automatically release the lock after this time # @param [Boolean] raise_on_failure Raise an error if a lock cannot be acquired, or fail silently # @raise [Mastodon::RaceConditionError] - def with_lock(lock_name, autorelease: 15.minutes, raise_on_failure: true) + def with_redis_lock(lock_name, autorelease: 15.minutes, raise_on_failure: true) with_redis do |redis| RedisLock.acquire(redis: redis, key: "lock:#{lock_name}", autorelease: autorelease.seconds) do |lock| if lock.acquired? diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 603e4cf48b..ca0083b167 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -24,7 +24,7 @@ class ActivityPub::ProcessAccountService < BaseService # The key does not need to be unguessable, it just needs to be somewhat unique @options[:request_id] ||= "#{Time.now.utc.to_i}-#{username}@#{domain}" - with_lock("process_account:#{@uri}") do + with_redis_lock("process_account:#{@uri}") do @account = Account.remote.find_by(uri: @uri) if @options[:only_key] @account ||= Account.find_remote(@username, @domain) @old_public_key = @account&.public_key diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index ac7372f745..38f6bf2514 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -35,7 +35,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService last_edit_date = @status.edited_at.presence || @status.created_at # Only allow processing one create/update per status at a time - with_lock("create:#{@uri}") do + with_redis_lock("create:#{@uri}") do Status.transaction do record_previous_edit! update_media_attachments! @@ -58,7 +58,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end def handle_implicit_update! - with_lock("create:#{@uri}") do + with_redis_lock("create:#{@uri}") do update_poll!(allow_significant_changes: false) queue_poll_notifications! end diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 8d07958b73..9c56c862ec 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -23,7 +23,7 @@ class FetchLinkCardService < BaseService @url = @original_url.to_s - with_lock("fetch:#{@original_url}") do + with_redis_lock("fetch:#{@original_url}") do @card = PreviewCard.find_by(url: @url) process_url if @card.nil? || @card.updated_at <= 2.weeks.ago || @card.missing_image? end diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index ea799db57f..25da2c6eb4 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -18,7 +18,7 @@ class RemoveStatusService < BaseService @account = status.account @options = options - with_lock("distribute:#{@status.id}") do + with_redis_lock("distribute:#{@status.id}") do @status.discard_with_reblogs StatusPin.find_by(status: @status)&.destroy diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index abe1534a55..870d67ec8d 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -106,7 +106,7 @@ class ResolveAccountService < BaseService def fetch_account! return unless activitypub_ready? - with_lock("resolve:#{@username}@#{@domain}") do + with_redis_lock("resolve:#{@username}@#{@domain}") do @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url, suppress_errors: @options[:suppress_errors]) end diff --git a/app/services/unfollow_service.rb b/app/services/unfollow_service.rb index d83a60e4e7..fe9a7f0d87 100644 --- a/app/services/unfollow_service.rb +++ b/app/services/unfollow_service.rb @@ -15,7 +15,7 @@ class UnfollowService < BaseService @target_account = target_account @options = options - with_lock("relationship:#{[source_account.id, target_account.id].sort.join(':')}") do + with_redis_lock("relationship:#{[source_account.id, target_account.id].sort.join(':')}") do unfollow! || undo_follow_request! end end diff --git a/app/services/vote_service.rb b/app/services/vote_service.rb index 9ebf5a98d9..3e92a1690a 100644 --- a/app/services/vote_service.rb +++ b/app/services/vote_service.rb @@ -18,7 +18,7 @@ class VoteService < BaseService already_voted = true - with_lock("vote:#{@poll.id}:#{@account.id}") do + with_redis_lock("vote:#{@poll.id}:#{@account.id}") do already_voted = @poll.votes.where(account: @account).exists? ApplicationRecord.transaction do diff --git a/app/workers/distribution_worker.rb b/app/workers/distribution_worker.rb index 59cdbc7b25..1d58e53e94 100644 --- a/app/workers/distribution_worker.rb +++ b/app/workers/distribution_worker.rb @@ -6,7 +6,7 @@ class DistributionWorker include Lockable def perform(status_id, options = {}) - with_lock("distribute:#{status_id}") do + with_redis_lock("distribute:#{status_id}") do FanOutOnWriteService.new.call(Status.find(status_id), **options.symbolize_keys) end rescue ActiveRecord::RecordNotFound