Skip to content

Commit 475877f

Browse files
byrootjanko
andcommitted
Always fully clear current attributes
Attempting to reset instances is a recipe for causing state leak. Regular instance variables in `Current` subclasses aren't cleared, so unless you remember to undefine them in the `reset` callback they will leak across requests. We could of course document that, but it's just too much of a footgun, and I see no benefit of trying to re-use that object instance. It's much safer to just always drop it and start fresh. Closes: rails#55125 Co-Authored-By: Janko Marohnić <[email protected]>
1 parent 8065028 commit 475877f

File tree

4 files changed

+20
-10
lines changed

4 files changed

+20
-10
lines changed

activesupport/CHANGELOG.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
* Always clear `CurrentAttribute` instances.
2+
3+
Previously `CurrentAttribute` instance would be reset at the end of requests.
4+
Meaning its attributes would be re-initialized.
5+
6+
This is problematic because it assume these objects don't hold any state
7+
other than their declared attribute, which isn't always the case, and
8+
can lead to state leak across request.
9+
10+
Now `CurrentAttribute` instances are abandonned at the end of a request,
11+
and a new instance is created at the start of the next request.
12+
13+
*Jean Boussier*, *Janko Marohnić*
14+
115
* Add public API for `before_fork_hook` in parallel testing.
216

317
Introduces a public API for calling the before fork hooks implemented by parallel testing.

activesupport/lib/active_support/current_attributes.rb

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,12 +153,8 @@ def resets(*methods, &block)
153153

154154
delegate :set, :reset, to: :instance
155155

156-
def reset_all # :nodoc:
157-
current_instances.each_value(&:reset)
158-
end
159-
160156
def clear_all # :nodoc:
161-
reset_all
157+
current_instances.each_value(&:reset)
162158
current_instances.clear
163159
end
164160

activesupport/lib/active_support/current_attributes/test_helper.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22

33
module ActiveSupport::CurrentAttributes::TestHelper # :nodoc:
44
def before_setup
5-
ActiveSupport::CurrentAttributes.reset_all
5+
ActiveSupport::CurrentAttributes.clear_all
66
super
77
end
88

99
def after_teardown
1010
super
11-
ActiveSupport::CurrentAttributes.reset_all
11+
ActiveSupport::CurrentAttributes.clear_all
1212
end
1313
end

activesupport/lib/active_support/railtie.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,10 @@ class Railtie < Rails::Railtie # :nodoc:
4444
app.executor.to_complete { ActiveSupport::ExecutionContext.clear }
4545
end
4646

47-
initializer "active_support.reset_all_current_attributes_instances" do |app|
47+
initializer "active_support.clear_all_current_attributes_instances" do |app|
4848
app.reloader.before_class_unload { ActiveSupport::CurrentAttributes.clear_all }
49-
app.executor.to_run { ActiveSupport::CurrentAttributes.reset_all }
50-
app.executor.to_complete { ActiveSupport::CurrentAttributes.reset_all }
49+
app.executor.to_run { ActiveSupport::CurrentAttributes.clear_all }
50+
app.executor.to_complete { ActiveSupport::CurrentAttributes.clear_all }
5151

5252
ActiveSupport.on_load(:active_support_test_case) do
5353
if app.config.active_support.executor_around_test_case

0 commit comments

Comments
 (0)