Rename `with_lock` to `with_redis_lock` to avoid confusion with ActiveRecord's method (#24741)

This commit is contained in:
Matt Jankowski 2023-05-02 12:16:07 -04:00 committed by GitHub
parent a7df578f97
commit f1c1dd0118
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 19 additions and 26 deletions

View File

@ -1410,13 +1410,6 @@ Rails/SkipsModelValidations:
- 'spec/services/follow_service_spec.rb' - 'spec/services/follow_service_spec.rb'
- 'spec/services/update_account_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. # Configuration parameters: Include.
# Include: app/models/**/*.rb # Include: app/models/**/*.rb
Rails/UniqueValidationWithoutIndex: Rails/UniqueValidationWithoutIndex:

View File

@ -16,7 +16,7 @@ class MediaProxyController < ApplicationController
rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error
def show 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]) @media_attachment = MediaAttachment.remote.attached.find(params[:id])
authorize @media_attachment.status, :show? authorize @media_attachment.status, :show?
redownload! if @media_attachment.needs_redownload? && !reject_media? redownload! if @media_attachment.needs_redownload? && !reject_media?

View File

@ -15,7 +15,7 @@ class Settings::ExportsController < Settings::BaseController
def create def create
backup = nil backup = nil
with_lock("backup:#{current_user.id}") do with_redis_lock("backup:#{current_user.id}") do
authorize :backup, :create? authorize :backup, :create?
backup = current_user.backups.create! backup = current_user.backups.create!
end end

View File

@ -4,7 +4,7 @@ class ActivityPub::Activity::Announce < ActivityPub::Activity
def perform def perform
return reject_payload! if delete_arrived_first?(@json['id']) || !related_to_local_activity? 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 original_status = status_from_object
return reject_payload! if original_status.nil? || !announceable?(original_status) return reject_payload! if original_status.nil? || !announceable?(original_status)

View File

@ -47,7 +47,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
def create_status def create_status
return reject_payload! if unsupported_object_type? || non_matching_uri_hosts?(@account.uri, object_uri) || tombstone_exists? || !related_to_local_activity? 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? return if delete_arrived_first?(object_uri) || poll_vote?
@status = find_existing_status @status = find_existing_status
@ -313,7 +313,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
poll = replied_to_status.preloadable_poll poll = replied_to_status.preloadable_poll
already_voted = true 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? already_voted = poll.votes.where(account: @account).exists?
poll.votes.create!(account: @account, choice: poll.options.index(@object['name']), uri: object_uri) poll.votes.create!(account: @account, choice: poll.options.index(@object['name']), uri: object_uri)
end end

View File

@ -12,7 +12,7 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
private private
def delete_person 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) DeleteAccountService.new.call(@account, reserve_username: false, skip_activitypub: true)
end end
end end
@ -20,14 +20,14 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity
def delete_note def delete_note
return if object_uri.nil? 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) unless non_matching_uri_hosts?(@account.uri, object_uri)
# This lock ensures a concurrent `ActivityPub::Activity::Create` either # This lock ensures a concurrent `ActivityPub::Activity::Create` either
# does not create a status at all, or has finished saving it to the # does not create a status at all, or has finished saving it to the
# database before we try to load it. # database before we try to load it.
# Without the lock, `delete_later!` could be called after `delete_arrived_first?` # Without the lock, `delete_later!` could be called after `delete_arrived_first?`
# and `Status.find` before `Status.create!` # 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) Tombstone.find_or_create_by(uri: object_uri, account: @account)
end end

View File

@ -42,7 +42,7 @@ class AccountMigration < ApplicationRecord
return false unless errors.empty? return false unless errors.empty?
with_lock("account_migration:#{account.id}") do with_redis_lock("account_migration:#{account.id}") do
save save
end end
end end

View File

@ -5,7 +5,7 @@ module Lockable
# @param [ActiveSupport::Duration] autorelease Automatically release the lock after this time # @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 # @param [Boolean] raise_on_failure Raise an error if a lock cannot be acquired, or fail silently
# @raise [Mastodon::RaceConditionError] # @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| with_redis do |redis|
RedisLock.acquire(redis: redis, key: "lock:#{lock_name}", autorelease: autorelease.seconds) do |lock| RedisLock.acquire(redis: redis, key: "lock:#{lock_name}", autorelease: autorelease.seconds) do |lock|
if lock.acquired? if lock.acquired?

View File

@ -24,7 +24,7 @@ class ActivityPub::ProcessAccountService < BaseService
# The key does not need to be unguessable, it just needs to be somewhat unique # 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}" @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.remote.find_by(uri: @uri) if @options[:only_key]
@account ||= Account.find_remote(@username, @domain) @account ||= Account.find_remote(@username, @domain)
@old_public_key = @account&.public_key @old_public_key = @account&.public_key

View File

@ -35,7 +35,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
last_edit_date = @status.edited_at.presence || @status.created_at last_edit_date = @status.edited_at.presence || @status.created_at
# Only allow processing one create/update per status at a time # 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 Status.transaction do
record_previous_edit! record_previous_edit!
update_media_attachments! update_media_attachments!
@ -58,7 +58,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
end end
def handle_implicit_update! def handle_implicit_update!
with_lock("create:#{@uri}") do with_redis_lock("create:#{@uri}") do
update_poll!(allow_significant_changes: false) update_poll!(allow_significant_changes: false)
queue_poll_notifications! queue_poll_notifications!
end end

View File

@ -23,7 +23,7 @@ class FetchLinkCardService < BaseService
@url = @original_url.to_s @url = @original_url.to_s
with_lock("fetch:#{@original_url}") do with_redis_lock("fetch:#{@original_url}") do
@card = PreviewCard.find_by(url: @url) @card = PreviewCard.find_by(url: @url)
process_url if @card.nil? || @card.updated_at <= 2.weeks.ago || @card.missing_image? process_url if @card.nil? || @card.updated_at <= 2.weeks.ago || @card.missing_image?
end end

View File

@ -18,7 +18,7 @@ class RemoveStatusService < BaseService
@account = status.account @account = status.account
@options = options @options = options
with_lock("distribute:#{@status.id}") do with_redis_lock("distribute:#{@status.id}") do
@status.discard_with_reblogs @status.discard_with_reblogs
StatusPin.find_by(status: @status)&.destroy StatusPin.find_by(status: @status)&.destroy

View File

@ -106,7 +106,7 @@ class ResolveAccountService < BaseService
def fetch_account! def fetch_account!
return unless activitypub_ready? 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]) @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url, suppress_errors: @options[:suppress_errors])
end end

View File

@ -15,7 +15,7 @@ class UnfollowService < BaseService
@target_account = target_account @target_account = target_account
@options = options @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! unfollow! || undo_follow_request!
end end
end end

View File

@ -18,7 +18,7 @@ class VoteService < BaseService
already_voted = true 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? already_voted = @poll.votes.where(account: @account).exists?
ApplicationRecord.transaction do ApplicationRecord.transaction do

View File

@ -6,7 +6,7 @@ class DistributionWorker
include Lockable include Lockable
def perform(status_id, options = {}) 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) FanOutOnWriteService.new.call(Status.find(status_id), **options.symbolize_keys)
end end
rescue ActiveRecord::RecordNotFound rescue ActiveRecord::RecordNotFound