-
Notifications
You must be signed in to change notification settings - Fork 354
Ractor-shareable JSON::Coder #897
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: master
Are you sure you want to change the base?
Ractor-shareable JSON::Coder #897
Conversation
|
CI needs some fixing but otherwise 👍 |
| }, | ||
| 0, 0, | ||
| RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED, | ||
| RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED | RUBY_TYPED_FROZEN_SHAREABLE, |
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.
This seems correct because all the fields in
json/ext/json/ext/parser/parser.c
Lines 331 to 342 in 7b1fb27
| typedef struct JSON_ParserStruct { | |
| VALUE on_load_proc; | |
| VALUE decimal_class; | |
| ID decimal_method_id; | |
| enum duplicate_key_action on_duplicate_key; | |
| int max_nesting; | |
| bool allow_nan; | |
| bool allow_trailing_comma; | |
| bool parsing_name; | |
| bool symbolize_names; | |
| bool freeze; | |
| } JSON_ParserConfig; |
are only written in
parser_config_init().However we need to make sure that's not called multiple times, so there should be a
rb_check_frozen() at the beginning of parser_config_init().
BTW bool parsing_name; seems unused.
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.
BTW
bool parsing_name;seems unused.
Good catch: ab5efca
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.
@eregon Thanks for the review! I added a call to rb_check_frozen() in cParserConfig_initialize.
>> JSON::Coder.new.freeze.instance_variable_get(:@parser_config).send(:initialize, {})
(irb):1:in 'JSON::Ext::ParserConfig#initialize': can't modify frozen JSON::Ext::ParserConfig: #<JSON::Ext::ParserConfig:0x0000000125000d40> (FrozenError)I wasn't sure whether to add a test for that, it looked like this:
coder = JSON::Coder.new.freeze
parser_config = coder.instance_variable_get(:@parser_config)
assert_instance_of JSON::Parser::Config, parser_config
assert_raise FrozenError do
parser_config.send(:initialize, {})
endBTW I just realized that the Generator::State object is already frozen in initialize, I'll actually do the same for the Parser::Config.
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'll also need to make sure a frozen Ext::Generator::State is not modified:
>> state = JSON::Coder.new.instance_variable_get(:@state)
=> #<JSON::Ext::Generator::State:0x000000012557c7d8>
>> state.frozen?
=> true
>> state.max_nesting
=> 100
>> state.max_nesting=1
=> 1
>> state.max_nesting
=> 1
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.
Good catch, that's already marked as RUBY_TYPED_FROZEN_SHAREABLE but it's not correct since it can still be mutated. So I guess we need rb_check_frozen() in all methods that can mutate a Ext::Generator::State then.
For Ext::Generator::State fields which don't contain a shareable value (e.g. strings) it would break the Ractor invariant (unshareable objects must always stay in a single Ractor) and probably segfault. And it shouldn't be visible as mutable anyway if marked as RUBY_TYPED_FROZEN_SHAREABLE.
77c03ae to
7ea7a05
Compare
|
I can't repro the errors with JRuby on my machine. I don't have the same JDK but still… vs. https://github.com/ruby/json/actions/runs/19504076937/job/55825471457#step:3:39 |
7ea7a05 to
76ed0f4
Compare
Weird, I can't either. |
|
It seems somewhat transient as it doesn't happen on all platforms. |
|
The path seems correct: /Users/runner/work/json/json/lib/json/ext/generator/state.rb
/Users/runner/work/json/json/lib/json/ext/generator.jarhttps://github.com/ruby/json/actions/runs/19541236496/job/55947865782#step:5:24 |
|
Maybe it’s not worth testing this closely anyways? |
|
Yeah at first I didn't have the test at all but then when I found out that the existing |
0c34822 to
7ca198c
Compare
|
I would suggest to keep the test (it was a pretty subtle and important bug) and skip it on JRuby, it sounds like a JRuby bug to me, maybe something @headius could investigate when he has some time. |
|
Re-reading the diff, the test for |
7ca198c to
1df7523
Compare
I want to be able to reuse an instance of
JSON::Coderacross Ractors. We need to add RUBY_TYPED_FROZEN_SHAREABLE to the parser config typed data definition and freeze the instance variables, after which it's possible to use a frozen coder across Ractors.