-
Notifications
You must be signed in to change notification settings - Fork 6
Introduce ractorize #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
95ad01b
26fed27
295761c
30e8227
aa7bc01
0c3e6a2
5e917e6
6cd73f6
ab8cd6b
75d3fe3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,16 @@ def initialize(parent = nil) | |
| end | ||
| end | ||
|
|
||
| def freeze | ||
| return self if frozen? | ||
|
|
||
| @own_keys = own_keys.dup.freeze | ||
| self.default_proc = nil | ||
| @parent.freeze | ||
| replace(to_h) | ||
| super | ||
| end | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks the contract of inheritable options. h=ActiveSupport::InheritableOptions.new(foo: 42)
h[:foo] # => 42
h.default_proc=nil
h[:foo] # => nilIIRC I had something like: def freeze
replace(to_h)
super
endBut that is a bit naive, trades memory for performance, we may want to go the opposite way and also make
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah sorry 🤦 This doesn't follow the pattern of the others my bad. I think freezing the I'll think about it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually if the So unless I'm missing something, I don't think there's a good way to make a Hash with a default proc shareable, even if the default proc could in theory be shareable.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I don't think we can do it in the initializer. People can mutate the parent of course, so we can't freeze it then. We could do: That seems to work. But to your other comment above, it feels like we need way to hook into make_shareable, since the behaviour really makes no sense as part of freeze. And actually itll break in ruby 3 if we do this since
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, for now I think
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry missed your earlier message cause we posted around same time. But yeah, making default proc shareable seems too weird, I think
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This thing has some APIs beyond a normal hash so I don't think its fair game to just Instead I think its safer to merge the parent's entries at freezing time then remove the default proc. That way it still quacks like an inheritable options in all ways.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, using inherited = {foo: 42, bar: 21}
options = ActiveSupport::InheritableOptions.new(inherited)
options.bar = 21
options.baz = 7
assert_equal 42, options.foo
refute options.overridden?(:foo)
assert options.overridden?(:bar)
refute options.overridden?(:baz)
refute options.key?(:qux)We'll need to keep a copy of the keys before freezing.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I was thinking about this last night........... I think I will keep track of the keys when we call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I'll pull this into its own PR since it's becoming a bit involved |
||
|
|
||
| def to_h | ||
| @parent.to_h.merge(self) | ||
| end | ||
|
|
@@ -120,13 +130,17 @@ def pretty_print(pp) | |
| pp.pp_hash(to_h) | ||
| end | ||
|
|
||
| alias_method :own_key?, :key? | ||
| private :own_key? | ||
|
|
||
| def key?(key) | ||
| super || @parent.key?(key) | ||
| end | ||
|
|
||
| alias_method :_own_keys, :keys | ||
| private :_own_keys | ||
|
|
||
| def keys | ||
| @parent.keys | super | ||
| end | ||
|
|
||
| def overridden?(key) | ||
| !!(@parent && @parent.key?(key) && own_key?(key.to_sym)) | ||
| end | ||
|
|
@@ -143,5 +157,14 @@ def each(&block) | |
| to_h.each(&block) | ||
| self | ||
| end | ||
|
|
||
| private | ||
| def own_key?(key) | ||
| own_keys.include?(key.to_sym) | ||
| end | ||
|
|
||
| def own_keys | ||
| @own_keys || _own_keys | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "isolation/abstract_unit" | ||
| require "active_support/testing/ractors_assertions" | ||
|
|
||
| if RUBY_VERSION >= "4.0" | ||
| module ApplicationTests | ||
| class RactorsTest < ActiveSupport::TestCase | ||
| include ActiveSupport::Testing::Isolation | ||
| include ActiveSupport::Testing::RactorsAssertions | ||
|
|
||
| def setup | ||
| build_app | ||
|
|
||
| add_to_env_config "production", "ActiveSupport::Ractors.unshareable_proc_action = :raise" | ||
|
|
||
| # Remove some defaults that are not compatible | ||
| add_to_env_config "production", "config.logger = ActiveSupport::Logger.new(nil)" | ||
| add_to_env_config "production", "config.public_file_server.enabled = false" | ||
| add_to_env_config "production", "config.cache_store = :null_store" | ||
| add_to_env_config "production", "config.action_cable.mount_path = nil" | ||
| end | ||
|
|
||
| def teardown | ||
| teardown_app | ||
| end | ||
|
|
||
| test "ractorize! makes the app shareable in production mode" do | ||
| app "production" | ||
|
|
||
| Rails.application.ractorize! | ||
|
|
||
| assert_ractor_shareable Rails.application | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also do
@memos.freezehere altho its not necessary when callingRactor.make_shareable, it seems weird to remove the default proc without freezing the hash.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's super weird and something
Ractor.make_shareablereally doesn't help with because it callsfreeze, checks that the object itself is frozen then moves on to freeze objects referenced by the object.On the one hand, it makes sense, because e.g.
[Object.new].freeze.first.frozen? # => false, on the other hand when designing classes it feels not helpful:There's no tool to let class authors know that
@foowasn't frozen, that would help them write a comprehensivefreezeimplementation.I'm not sure what it should look like though, maybe
Ractor.make_shareable(obj, freeze: false)that would raise if any referenced object is not already frozen maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call freeze. I know it's not necessary if you're going to turn around and call
make_shareable, but I usually expect callingfreezeon a complex object to freeze its references.@etiennebarrie Could you just try passing the frozen object to a Ractor and see if it raises an exception? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍 I made this and the others freeze all their internals.