Use a T_DATA for cross_ractor_require #13531
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
it should be problem with ractor local gc.
hmm, other ideas? |
paracycle
reviewed
Jun 6, 2025
Comment on lines
+2240
to
+2253
| static void | ||
| cross_ractor_require_mark(void *ptr) | ||
| { | ||
| struct cross_ractor_require *crr = (struct cross_ractor_require *)ptr; | ||
| rb_gc_mark(crr->port); | ||
| rb_gc_mark(crr->result); | ||
| rb_gc_mark(crr->exception); | ||
| rb_gc_mark(crr->feature); | ||
| rb_gc_mark(crr->module); | ||
| } |
There was a problem hiding this comment.
Should we use declarative marking for this instead? https://github.com/Shopify/ruby/blob/master/doc/extension.rdoc/#declaratively-markingcompacting-struct-references-
Previously rb_ractor_interrupt_exec would use an intermediate function to create a new thread with the actual target function, replacing the data being passed in with a piece of malloc memory holding the "next" function and the original data. Because of this, passing rb_interrupt_exec_flag_value_data to rb_ractor_interrupt_exec didn't have the intended effect of allowing data to be passed in and marked. This commit adds a rb_interrupt_exec_flag_new_thread flag, which both simplifies the implementation and allows the original data to be marked.
[Bug #21090]
The struct was previously allocated on the stack, which could be freed
if the Thread is terminated. Moving this to a T_DATA on the heap should
mean this is no longer an issue.
1000.times { Ractor.new { th = Thread.new { require "rbconfig" }; Thread.pass }.take }
Co-authored-by: Luke Gruber <luke.gruber@shopify.com>
cb5120c to
750e603
Compare
750e603 to
fc54ae5
Compare
This comment has been minimized.
This comment has been minimized.
|
I made the new CRR object shareable. I also fixed the todo comment and make |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes Bug #21090
The struct was previously allocated on the stack, which could be freed if the Thread is terminated. Moving this to a T_DATA on the heap should mean this is no longer an issue.
Previously this would segfault, due to accessing the Thread's memory after it was terminated (when the Ractor exits). Now it prints 1000 times:
The error comes from trying to send the result of the require back through the port. Maybe we could have a better error message, but that seems pretty reasonable to me given how much of an edge case this is.
cc @ko1 @luke-gru