Skip to content

Commit 1cd5ea3

Browse files
committed
Fix lost thread and fiber locals
Thread and fiber locals are being cleared on completion of every future. If the promise ran on the same thread as the request, or the same thread as the last promise, the locals are then not available. The execution context hasn't changed in those circumstances so the code that sets the locals at the start of a promise doesn't run. 1. Thread 1 runs 2. Sets context 3. Loads scope 4. Clears context 5. Thread 1 runs next scope 6. Context hasn’t changed so doesn’t set it again 7. Loads scope 8. Fails because the code relying on the context causes an exception because the locals aren't available. Instead always set the thread and fiber locals and only clear those that we know are new. This should avoid the lost locals and also avoid memory leaks between promises. See: graphiti-api#496
1 parent e5b54b6 commit 1cd5ea3

File tree

1 file changed

+34
-17
lines changed

1 file changed

+34
-17
lines changed

lib/graphiti/scope.rb

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ def future_with_context(*args)
140140
end
141141
fiber_storage =
142142
if Fiber.current.respond_to?(:storage)
143-
Fiber.current&.storage&.keys&.each_with_object({}) do |key, memo|
143+
Fiber.current.storage&.keys&.each_with_object({}) do |key, memo|
144144
memo[key] = Fiber[key]
145145
end
146146
end
@@ -149,27 +149,44 @@ def future_with_context(*args)
149149
self.class.global_thread_pool_executor, Thread.current.object_id, thread_storage, fiber_storage, *args
150150
) do |thread_id, thread_storage, fiber_storage, *args|
151151
wrap_in_rails_executor do
152-
execution_context_changed = thread_id != Thread.current.object_id
153-
if execution_context_changed
154-
thread_storage&.keys&.each_with_object(Thread.current) do |key, thread_current|
155-
thread_current[key] = thread_storage[key]
152+
with_thread_locals(thread_storage) do
153+
with_fiber_locals(fiber_storage) do
154+
Graphiti.broadcast(:global_thread_pool_task_run, self.class.global_thread_pool_stats) do
155+
yield(*args)
156+
end
156157
end
157-
fiber_storage&.keys&.each_with_object(Fiber) do |key, fiber_current|
158-
fiber_current[key] = fiber_storage[key]
159-
end
160-
end
161-
162-
result = Graphiti.broadcast(:global_thread_pool_task_run, self.class.global_thread_pool_stats) do
163-
yield(*args)
164158
end
159+
end
160+
end
161+
end
165162

166-
if execution_context_changed
167-
thread_storage&.keys&.each { |key| Thread.current[key] = nil }
168-
fiber_storage&.keys&.each { |key| Fiber[key] = nil }
169-
end
163+
def with_thread_locals(thread_locals)
164+
new_thread_locals = []
165+
thread_locals.each do |key, value|
166+
if !Thread.current[key]
167+
new_thread_locals << key
168+
end
169+
Thread.current[key] = value
170+
end
171+
yield
172+
ensure
173+
new_thread_locals.each do |key|
174+
Thread.current[key] = nil
175+
end
176+
end
170177

171-
result
178+
def with_fiber_locals(fiber_locals)
179+
new_fiber_locals = []
180+
fiber_locals.each do |key, value|
181+
if !Fiber[key]
182+
new_fiber_locals << key
172183
end
184+
Fiber[key] = value
185+
end
186+
yield
187+
ensure
188+
new_fiber_locals.each do |key|
189+
Fiber[key] = nil
173190
end
174191
end
175192

0 commit comments

Comments
 (0)