Skip to content

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

Merged
sporkmonger merged 4 commits into
sporkmonger:mainfrom
okeeblow:freeze_concatenated_strings
Jul 30, 2022
Merged

Explicitly freeze concatenated-String constants to unbreak on non-main Ractors.#449
sporkmonger merged 4 commits into
sporkmonger:mainfrom
okeeblow:freeze_concatenated_strings

Conversation

@okeeblow

@okeeblow okeeblow commented Jan 15, 2022

Copy link
Copy Markdown
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>

houndci-bot
houndci-bot reviewed Jan 15, 2022
View reviewed changes
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
Comment thread lib/addressable/uri.rb Outdated
@dentarg

dentarg commented Jan 15, 2022

Copy link
Copy Markdown
Collaborator

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
okeeblow force-pushed the freeze_concatenated_strings branch from 37d2285 to 0f5c03d Compare January 15, 2022 21:04
@okeeblow

okeeblow commented Jan 15, 2022

Copy link
Copy Markdown
Contributor Author

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

@dentarg

dentarg commented Jan 15, 2022

Copy link
Copy Markdown
Collaborator

Can you add a test too?

@okeeblow
okeeblow force-pushed the freeze_concatenated_strings branch from 0f5c03d to 6390312 Compare January 22, 2022 22:29
houndci-bot
houndci-bot reviewed Jan 22, 2022
View reviewed changes
Comment thread spec/addressable/uri_spec.rb
@dentarg

dentarg commented Jan 22, 2022

Copy link
Copy Markdown
Collaborator

Can you resolve the conflict?

dentarg
dentarg reviewed Jan 22, 2022
View reviewed changes
Comment thread spec/addressable/uri_spec.rb Outdated
@okeeblow
okeeblow force-pushed the freeze_concatenated_strings branch from 6390312 to 178bcd9 Compare January 23, 2022 01:02
@sporkmonger
sporkmonger merged commit 7ecf751 into sporkmonger:main Jul 30, 2022
@okeeblow
okeeblow deleted the freeze_concatenated_strings branch September 24, 2022 23:06
@dentarg dentarg mentioned this pull request Feb 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dentarg dentarg dentarg left review comments
@houndci-bot houndci-bot houndci-bot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@okeeblow @dentarg @houndci-bot @sporkmonger