Draft: Fix intermittent crashes with GC.compact #171
Closed
Conversation
This file contains 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
This fixes the known issue with compaction as described in https://bugs.ruby-lang.org/issues/15626#Known-Issue. Since Hamlit holds points to references to VALUE pointers, these VALUE pointers could move due to compaction. To avoid this, we pin these objects with `rb_gc_mark`.
stanhu
changed the title
|
Hmm, this crashes Ruby 3.0 indeed. |
I wonder if this is related to Ractors: https://github.com/ruby/ruby/blob/ruby_3_0/gc.c#L10166-L10171 This is crashing in libpthread |
|
I wonder as well: I believe marking is done recursively, starting with root objects. These objects here are modules. Perhaps we shouldn't explicitly mark the submodules. Could we try to only mark |
Good idea. Let me revise. I think this still crashes in Ruby 3.0, though. |
|
made objects by |
`rb_define_module_under()` are already pinned.
|
I'm still seeing this failure in my reproduction script: |
|
Hmm, are modules defined via |
|
Running the script a few times on Linux yields this: |
|
Is the actual issue here the |
|
I successfully reproduced the issue with your script, but after I rebuilt the gem, it never happened again... It'd be really helpful if we can somehow increase the possibility of reproduction, because I cannot verify any fix on my end anymore. But this output #171 (comment) is really helpful. It shows inlined frames and line numbers of Hamlit unlike #171 (comment). It's unfortunate that this is not Ruby 3.0's but Ruby 2.7's though. According to the backtrace, the problem seems to happen in |
|
This is very hard to fix on my end because your script doesn't reproduce the issue on my environment (Linux x86_64) anymore. I've reproduced only once so far. ko1 advised me to check if diff --git a/ext/hamlit/hamlit.c b/ext/hamlit/hamlit.c
index 11efb33..d883678 100644
--- a/ext/hamlit/hamlit.c
+++ b/ext/hamlit/hamlit.c
@@ -9,12 +9,12 @@ static ID id_flatten, id_keys, id_parse, id_prepend, id_tr, id_uniq_bang;
static ID id_aria, id_data, id_equal, id_hyphen, id_space, id_underscore;
static ID id_boolean_attributes, id_xhtml;
-static VALUE str_aria() { return rb_const_get(mAttributeBuilder, id_aria); }
-static VALUE str_data() { return rb_const_get(mAttributeBuilder, id_data); }
-static VALUE str_equal() { return rb_const_get(mAttributeBuilder, id_equal); }
-static VALUE str_hyphen() { return rb_const_get(mAttributeBuilder, id_hyphen); }
-static VALUE str_space() { return rb_const_get(mAttributeBuilder, id_space); }
-static VALUE str_underscore() { return rb_const_get(mAttributeBuilder, id_underscore); }
+static VALUE str_aria() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_aria); }
+static VALUE str_data() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_data); }
+static VALUE str_equal() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_equal); }
+static VALUE str_hyphen() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_hyphen); }
+static VALUE str_space() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_space); }
+static VALUE str_underscore() { fprintf(stderr, "%d", RB_TYPE_P(mAttributeBuilder, T_MOVED)); return rb_const_get(mAttributeBuilder, id_underscore); }
static void
delete_falsey_values(VALUE values) |
|
It returns |
|
I also checked |
|
It seems that I can easily reproduce this with Ruby 2.7.2. I'll take a look at the details. |
|
I've failed to find anything moved. By the way, is it possible to reproduce this issue without |
|
It's still not clear why it's happening, but I found one workaround. I'm implementing it. |
|
Thanks! I'll close this pull request then. I was able to reproduce this without the |
k0kubun
added a commit
that referenced
this pull request
Jan 21, 2021
to workaround SEGV after GC.compact. Fix #171
k0kubun
added a commit
that referenced
this pull request
Jan 21, 2021
to workaround SEGV after GC.compact. Fix #171
k0kubun
added a commit
that referenced
this pull request
Jan 21, 2021
to workaround SEGV after GC.compact. Fix #171
|
Could you check if #172 works? I'll release this tomorrow if it works out. |
|
Let me give it a shot. FYI, I haven't yet been able to reproduce the failure with this patch. Note I first tried marking the result of the diff --git a/ext/hamlit/hamlit.c b/ext/hamlit/hamlit.c
index 11efb33..4ef812c 100644
--- a/ext/hamlit/hamlit.c
+++ b/ext/hamlit/hamlit.c
@@ -510,6 +510,16 @@ rb_hamlit_build(int argc, VALUE *argv, RB_UNUSED_VAR(VALUE self))
return hamlit_build(argv[0], argv[1], argv[2], argv[3], array);
}
+void
+set_attribute_builder_const(ID id, const char *name)
+{
+ VALUE str = rb_str_new_cstr(name);
+ VALUE obj = rb_obj_freeze(str);
+
+ rb_gc_register_mark_object(str);
+ rb_const_set(mAttributeBuilder, id, obj);
+}
+
void
Init_hamlit(void)
{
@@ -544,11 +554,11 @@ Init_hamlit(void)
id_boolean_attributes = rb_intern("BOOLEAN_ATTRIBUTES");
id_xhtml = rb_intern("xhtml");
- rb_const_set(mAttributeBuilder, id_aria, rb_obj_freeze(rb_str_new_cstr("aria")));
- rb_const_set(mAttributeBuilder, id_data, rb_obj_freeze(rb_str_new_cstr("data")));
- rb_const_set(mAttributeBuilder, id_equal, rb_obj_freeze(rb_str_new_cstr("=")));
- rb_const_set(mAttributeBuilder, id_hyphen, rb_obj_freeze(rb_str_new_cstr("-")));
- rb_const_set(mAttributeBuilder, id_space, rb_obj_freeze(rb_str_new_cstr(" ")));
- rb_const_set(mAttributeBuilder, id_underscore, rb_obj_freeze(rb_str_new_cstr("_")));
+ set_attribute_builder_const(id_aria, "aria");
+ set_attribute_builder_const(id_data, "data");
+ set_attribute_builder_const(id_equal, "=");
+ set_attribute_builder_const(id_hyphen, "-");
+ set_attribute_builder_const(id_space, " ");
+ set_attribute_builder_const(id_underscore, "_");
}
#endif |
|
Eh, never mind, it just failed. Let me try your patch. |
|
#172 appears to work well for me. Thank you! |
|
Thanks for checking it quickly. I released it as v2.14.2. |
stanhu
added a commit
to stanhu/hamlit
that referenced
this pull request
Mar 18, 2021
This would have detected k0kubun#177 and k0kubun#171 in Ruby 3.0.
Closed
stanhu
added a commit
to stanhu/hamlit
that referenced
this pull request
Mar 18, 2021
This would have detected k0kubun#177 and k0kubun#171 in Ruby 3.0.
stanhu
added a commit
to stanhu/hamlit
that referenced
this pull request
Mar 18, 2021
This would have detected k0kubun#177 and k0kubun#171 in Ruby 3.0.
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.
This fixes the known issue with compaction as described in
https://bugs.ruby-lang.org/issues/15626#Known-Issue.
Since Hamlit holds points to references to VALUE pointers, these
VALUE pointers could move due to compaction. To avoid this, we
pin these objects with
rb_gc_mark.I was able to reproduce this issue intermittently via: