Project

General

Profile

Actions

Feature #15974

closed

Warn in verbose mode on defining a finalizer that captures the object

Feature #15974: Warn in verbose mode on defining a finalizer that captures the object

Added by chrisseaton (Chris Seaton) over 6 years ago. Updated about 5 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:93485]

Description

There's a really common mistake people make when using define_finalizer - they capture the object in the finalizer.

https://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/

This PR warns about that, when in verbose mode (it's too slow to do always.)

https://github.com/ruby/ruby/pull/2264

Updated by ko1 (Koichi Sasada) over 6 years ago Actions #1 [ruby-core:93489]

it makes sense.

Updated by mame (Yusuke Endoh) over 6 years ago Actions #2 [ruby-core:93493]

Matz has approved this proposal.

Updated by shevegen (Robert A. Heiler) over 6 years ago Actions #3 [ruby-core:93496]

Matz has approved this proposal.

\o/

Updated by nobu (Nobuyoshi Nakada) over 6 years ago Actions #4 [ruby-core:93497]

This is a good feature, but I don't think this should belong to the "spec" as seems an implementation detail.

Updated by chrisseaton (Chris Seaton) over 6 years ago Actions #5 [ruby-core:93499]

I think Ruby Spec considers warnings to be user-visible features, so does normally spec them. This prompts alternative implementations like JRuby and TruffleRuby to implement the same warning so that the user experience is the same on all implementations.

@Eregon (Benoit Daloze) will be able to give a definitive opinion on whether it should be spec'd or not.

Updated by chrisseaton (Chris Seaton) over 6 years ago Actions #6

  • Status changed from Open to Closed

Applied in changeset git|928260c2a613bbdd4402c300e0bf86ae7562e52a.


Warn in verbose mode on defining a finalizer that captures the object

[Feature #15974]

Closes: https://github.com/ruby/ruby/pull/2264

Updated by Eregon (Benoit Daloze) over 6 years ago Actions #7 [ruby-core:93500]

chrisseaton (Chris Seaton) wrote:

I think Ruby Spec considers warnings to be user-visible features, so does normally spec them. This prompts alternative implementations like JRuby and TruffleRuby to implement the same warning so that the user experience is the same on all implementations.

@Eregon (Benoit Daloze) will be able to give a definitive opinion on whether it should be spec'd or not.

Yes, exactly, it's a useful feature for other implementations to have too and it's user-visible so it's good to add to ruby/spec, similar to warnings.

Updated by Dan0042 (Daniel DeLorme) about 6 years ago Actions #8 [ruby-core:94166]

Question: shouldn't this be an error rather than a warning? Defining a finalizer that immortalizes its object and can never run seems like an outright bug, not something that you should merely warn about in verbose mode only.

Updated by chrisseaton (Chris Seaton) about 6 years ago Actions #9 [ruby-core:94167]

Question: shouldn't this be an error rather than a warning?

No, because it's possible to have a reference from the finaliser to the object when the finaliser is created, but then to later clear that reference and the finaliser to work properly. The specs cover this.

However this PR was reverted due to some bugs (I wrote it in a couple of hours at a hackathon and it was merged then and there, so not surprising it wasn't really fully baked yet!) I hope to get it back in at some point.

Updated by Dan0042 (Daniel DeLorme) about 6 years ago Actions #10 [ruby-core:94168]

Ah, good point.

But maybe ObjectSpace.define_finalizer(obj, aProc) should raise an error if aProc.binding.receiver.equal?(obj), because that's a reference that can never be cleared.

Updated by chrisseaton (Chris Seaton) about 6 years ago Actions #11 [ruby-core:94169]

Yes we could do that, and enable it always as that'd be cheap.

Updated by chrisseaton (Chris Seaton) about 5 years ago Actions #12 [ruby-core:99670]

A partial solution to this but with a better implementation is at https://github.com/ruby/ruby/pull/3444.

Actions

Also available in: PDF Atom