Skip to content

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

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

chrisseaton
Copy link
Contributor

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:

class CapturesSelf
  def initialize(name)
    ObjectSpace.define_finalizer(self, proc {
      # this finalizer will only be run on exit because it has captured self
      puts "finalizing #{name}"
    })
  end
end

CapturesSelf.new('foo')
./test.rb:3: warning: finalizer references object to be finalized

Also improve specs and documentation for finalizers and more clearly
recommend a safe code pattern to use them.
@chrisseaton chrisseaton force-pushed the finalizer-capture-warning branch from 8082be0 to e2967ca Compare September 16, 2020 18:59
@tenderlove tenderlove merged commit 8e173d8 into ruby:master Sep 16, 2020
VALUE
rb_callable_receiver(VALUE callable) {
if (rb_obj_is_proc(callable)) {
VALUE binding = rb_funcall(callable, rb_intern("binding"), 0);
Copy link
Contributor

@ko1 ko1 Sep 16, 2020

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.

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@chrisseaton chrisseaton deleted the finalizer-capture-warning branch October 5, 2020 22:45
@chrisseaton
Copy link
Contributor Author

@ko1 I think both your comments are hopefully addressed in #3629.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants