Skip to content
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

Explicitly freeze concatenated-String constants to unbreak on non-main Ractors. #449

Merged
merged 4 commits into from Jul 30, 2022

Conversation

okeeblow
Copy link
Contributor

When building Strings with the String#concat / String#+ methods, the receiver and argument Strings are literals, but the resulting value is not, so they are left unfrozen when we rely only on the frozen_string_literal directive.

Switch to the %q non-interpolable String literal syntax for these because it feels less gross to me than e.g. HOST = (UNRESERVED + SUB_DELIMS + "\\[\\:\\]").freeze. This syntax was present in Ruby 2.0, which is Addressable's minimum requirement :)

Before

Concatenated Strings are not frozen when the frozen_string_literal: true directive is used.

[okeeblow@emi#CHECKING YOU OUT] ruby2.7 -r 'addressable' -e 'Addressable::URI::CharacterClasses::constants::each { p "#{_1} frozen? #{Addressable::URI::CharacterClasses::const_get(_1).frozen?}" }'
"ALPHA frozen? true"
"DIGIT frozen? true"
"GEN_DELIMS frozen? true"
"SUB_DELIMS frozen? true"
"RESERVED frozen? false"
"UNRESERVED frozen? false"
"PCHAR frozen? false"
"SCHEME frozen? false"
"HOST frozen? false"
"AUTHORITY frozen? false"
"PATH frozen? false"
"QUERY frozen? false"
"FRAGMENT frozen? false"

…and we encounter a Ractor::IsolationError when trying to use Addressable::URI::parse in a non-main Ractor:

[okeeblow@emi#CHECKING YOU OUT] ruby -v
ruby 3.1.0p0 (2021-12-25 revision fb4df44d16) [x86_64-linux]

[okeeblow@emi#CHECKING YOU OUT] ruby -r 'addressable' -e 'Addressable::URI::CharacterClasses::constants::each { value = Addressable::URI::CharacterClasses::const_get(_1); p "#{_1} frozen? #{_1.frozen?}; shareable? #{Ractor::shareable?(value)}"}'
"ALPHA frozen? true; shareable? true"
"DIGIT frozen? true; shareable? true"
"GEN_DELIMS frozen? true; shareable? true"
"SUB_DELIMS frozen? true; shareable? true"
"RESERVED frozen? true; shareable? false"
"UNRESERVED frozen? true; shareable? false"
"PCHAR frozen? true; shareable? false"
"SCHEME frozen? true; shareable? false"
"HOST frozen? true; shareable? false"
"AUTHORITY frozen? true; shareable? false"
"PATH frozen? true; shareable? false"
"QUERY frozen? true; shareable? false"
"FRAGMENT frozen? true; shareable? false"
irb(main):070:0> Ractor::new { Addressable::URI::parse('https://cooltrainer.org') }
#<Thread:0x00007f1e35a2e0e0 run> terminated with exception (report_on_exception is true):
/home/okeeblow/.gems/gems/addressable-2.8.0/lib/addressable/uri.rb:2497:in `validate': can not access non-shareable objects in constant Addressable::URI::CharacterClasses::UNRESERVED by non-main Ractor. (Ractor::IsolationError)                                               
        from /home/okeeblow/.gems/gems/addressable-2.8.0/lib/addressable/uri.rb:2421:in `defer_validation'
        from /home/okeeblow/.gems/gems/addressable-2.8.0/lib/addressable/uri.rb:840:in `initialize'
        from /home/okeeblow/.gems/gems/addressable-2.8.0/lib/addressable/uri.rb:147:in `new'
        from /home/okeeblow/.gems/gems/addressable-2.8.0/lib/addressable/uri.rb:147:in `parse'
        from (irb):70:in `block in <top (required)>'
=> #<Ractor:#11 (irb):70 terminated>

After

Strings are frozen, and Addressable can be used in a non-main Ractor:

[okeeblow@emi#CHECKING YOU OUT] ruby -v
ruby 3.1.0p0 (2021-12-25 revision fb4df44d16) [x86_64-linux]

[okeeblow@emi#CHECKING YOU OUT] bundle exec ruby -r 'addressable' -e 'Addressable::URI::CharacterClasses::constants::each { value = Addressable::URI::CharacterClasses::const_get(_1); p "#{_1} frozen? #{_1.frozen?}; shareable? #{Ractor::shareable?(value)}"}'
"ALPHA frozen? true; shareable? true"
"DIGIT frozen? true; shareable? true"
"GEN_DELIMS frozen? true; shareable? true"
"SUB_DELIMS frozen? true; shareable? true"
"RESERVED frozen? true; shareable? false"
"UNRESERVED frozen? true; shareable? false"
"PCHAR frozen? true; shareable? false"
"SCHEME frozen? true; shareable? false"
"HOST frozen? true; shareable? false"
"AUTHORITY frozen? true; shareable? false"
"PATH frozen? true; shareable? false"
"QUERY frozen? true; shareable? false"
"FRAGMENT frozen? true; shareable? false"

Even though Ractor::shareable? still returns false here, the Ractor "shareable objects" docs say frozen String objects are shareable. We could additionally call Ractor::make_shareable, but there seems to be no reason to since it does work:

[okeeblow@emi#CHECKING YOU OUT] ./bin/repl
irb(main):001:0> Ractor::new { Addressable::URI::parse('https://cooltrainer.org') }.take
=> #<Addressable::URI:0x1ae0 URI:https://cooltrainer.org>

lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
lib/addressable/uri.rb Outdated Show resolved Hide resolved
@dentarg
Copy link
Collaborator

dentarg commented Jan 15, 2022

Switch to the %q non-interpolable String literal syntax for these because it feels less gross to me than e.g. HOST = (UNRESERVED + SUB_DELIMS + "\\[\\:\\]").freeze

Well, it also needs to work. Did you not try out your change? :)

irb(main):001:0> FOO = "this is foo"
=> "this is foo"
irb(main):002:0> %q{FOO}
=> "FOO"
irb(main):003:0> %q{#{FOO}}
=> "\#{FOO}"

I guess you wanted %Q (aliased to %)?

irb(main):006:0> %{#{FOO}}.freeze
=> "this is foo"
irb(main):007:0> %{#{FOO}}.freeze.frozen?
=> true

but I think HOST = (UNRESERVED + SUB_DELIMS).freeze would be more clear

@okeeblow
Copy link
Contributor Author

Sure, that syntax is fine. I just want this gem to stop blowing up my entire app :)

@dentarg
Copy link
Collaborator

dentarg commented Jan 15, 2022

Can you add a test too?

@dentarg
Copy link
Collaborator

dentarg commented Jan 22, 2022

Can you resolve the conflict?

@sporkmonger sporkmonger merged commit 7ecf751 into sporkmonger:main Jul 30, 2022
@okeeblow okeeblow deleted the freeze_concatenated_strings branch September 24, 2022 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants