-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Finalizer capture warning #3444
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
Conversation
Also improve specs and documentation for finalizers and more clearly recommend a safe code pattern to use them.
8082be0
to
e2967ca
Compare
VALUE | ||
rb_callable_receiver(VALUE callable) { | ||
if (rb_obj_is_proc(callable)) { | ||
VALUE binding = rb_funcall(callable, rb_intern("binding"), 0); |
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.
please do not depend on Proc#binding because it can be limited in a future.
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.
Hmm not sure why I didn't call the methods directly since it's in the same compilation unit. I'll try that.
@@ -46,6 +46,7 @@ VALUE rb_method_call_with_block(int, const VALUE *, VALUE, VALUE); | |||
VALUE rb_method_call_with_block_kw(int, const VALUE *, VALUE, VALUE, int); | |||
int rb_mod_method_arity(VALUE, ID); | |||
int rb_obj_method_arity(VALUE, ID); | |||
VALUE rb_callable_receiver(VALUE); |
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.
do not add public API without any discussion.
At least, there is no rb_callable
methods in public C-API.
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.
Oh, sorry. I thought this was an 'internal' API - that's where the header is?
Or do you mean the symbol is literally visible after linking?
Do you want to revert the merge and let me find a way to fix it from where?
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.
now it is confusing.
RBIMPL_SYMBOL_EXPORT_BEGIN()
// functions are exposed
RBIMPL_SYMBOL_EXPORT_END()
maybe toplevel/internal/xxx will be a good place.
Feature https://bugs.ruby-lang.org/issues/15974.
The idea of this feature is to warn when a finalizer captures the object to be finalized, because this prevents the finalizer being run until the process exits, which is likely not what is intended. This PR covers the most simple and common case, where the receiver of the finalizer is the object to be finalized.
It's easy to do this https://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/ and I've done it myself before.
I originally implemented this feature in #2264, but there I tired to solve the general case of warning if the finalizer transitively captured the object to be finalized, by using the GC's mechanism to walk the object graph. That's useful, but impactical. This PR is simple, easy, can always be on, and will capture what I would guess is the most common case. The original implementation was also part of a hack-day, so was very rough, which is why it got reverted in 23c92b6, which was understandable.
I've also improved the specification and documentation of
ObjectSpace.define_finalizer
.Overall, this is the idea: