mirror of
https://github.com/discourse/discourse.git
synced 2025-06-04 07:34:40 +08:00
DEV: Make Ruby services thread-safe
A previous refactor of the `Service::Base::Step` class introduced a non thread-safe behavior. `#call` mutates instance variables at runtime, and since a step instance is the same for any given service class, this can sometimes lead to `context` being the wrong one for the running service. This patch makes use of `Concurrent::ThreadLocalVar` to fix the issue.
This commit is contained in:

committed by
Loïc Guitaut

parent
87f8845940
commit
a4d34d60e3
@ -129,16 +129,16 @@ module Service
|
|||||||
|
|
||||||
# @!visibility private
|
# @!visibility private
|
||||||
class Step
|
class Step
|
||||||
attr_reader :name, :method_name, :class_name, :instance, :context
|
attr_reader :name, :method_name, :class_name
|
||||||
|
|
||||||
def initialize(name, method_name = name, class_name: nil)
|
def initialize(name, method_name = name, class_name: nil)
|
||||||
@name = name
|
@name, @method_name, @class_name = name, method_name, class_name
|
||||||
@method_name = method_name
|
@instance = Concurrent::ThreadLocalVar.new
|
||||||
@class_name = class_name
|
@context = Concurrent::ThreadLocalVar.new
|
||||||
end
|
end
|
||||||
|
|
||||||
def call(instance, context)
|
def call(instance, context)
|
||||||
@instance, @context = instance, context
|
@instance.value, @context.value = instance, context
|
||||||
context[result_key] = Context.build
|
context[result_key] = Context.build
|
||||||
with_runtime { run_step }
|
with_runtime { run_step }
|
||||||
end
|
end
|
||||||
@ -147,6 +147,10 @@ module Service
|
|||||||
"result.#{type}.#{name}"
|
"result.#{type}.#{name}"
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def instance = @instance.value
|
||||||
|
|
||||||
|
def context = @context.value
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def run_step
|
def run_step
|
||||||
@ -252,6 +256,7 @@ module Service
|
|||||||
attr_reader :steps
|
attr_reader :steps
|
||||||
|
|
||||||
def initialize(&block)
|
def initialize(&block)
|
||||||
|
super("")
|
||||||
@steps = []
|
@steps = []
|
||||||
instance_exec(&block)
|
instance_exec(&block)
|
||||||
end
|
end
|
||||||
@ -268,8 +273,8 @@ module Service
|
|||||||
attr_reader :steps, :keys
|
attr_reader :steps, :keys
|
||||||
|
|
||||||
def initialize(*keys, &block)
|
def initialize(*keys, &block)
|
||||||
|
super(keys.join(":"))
|
||||||
@keys = keys
|
@keys = keys
|
||||||
@name = keys.join(":")
|
|
||||||
@steps = []
|
@steps = []
|
||||||
instance_exec(&block)
|
instance_exec(&block)
|
||||||
end
|
end
|
||||||
@ -308,7 +313,7 @@ module Service
|
|||||||
attr_reader :steps, :exceptions
|
attr_reader :steps, :exceptions
|
||||||
|
|
||||||
def initialize(exceptions, &block)
|
def initialize(exceptions, &block)
|
||||||
@name = "default"
|
super("default")
|
||||||
@steps = []
|
@steps = []
|
||||||
@exceptions = exceptions.presence || [StandardError]
|
@exceptions = exceptions.presence || [StandardError]
|
||||||
instance_exec(&block)
|
instance_exec(&block)
|
||||||
|
@ -122,4 +122,31 @@ RSpec.describe Service do
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe "Thread safety" do
|
||||||
|
context "when services are run concurrently" do
|
||||||
|
let(:service) do
|
||||||
|
Class.new do
|
||||||
|
include Service::Base
|
||||||
|
|
||||||
|
policy :limit_reached
|
||||||
|
step :update
|
||||||
|
|
||||||
|
def limit_reached
|
||||||
|
$redis.get("my_counter").to_i < 3
|
||||||
|
end
|
||||||
|
|
||||||
|
def update
|
||||||
|
$redis.incr("my_counter")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
it "works properly" do
|
||||||
|
# When not thread-safe, a failed service can hold the wrong context,
|
||||||
|
# which raises an exception
|
||||||
|
expect { 10.times.map { Thread.new { service.call } }.map(&:join) }.not_to raise_error
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
Reference in New Issue
Block a user