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

• Status: Closed
• Assignee: -
• Target version: -

[chrisseaton (Chris Seaton)]

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

[ko1 (Koichi Sasada)]

it makes sense.

[mame (Yusuke Endoh)]

Matz has approved this proposal.

[shevegen (Robert A. Heiler)]

Matz has approved this proposal.

\o/

[nobu (Nobuyoshi Nakada)]

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

[chrisseaton (Chris Seaton)]

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.

[chrisseaton (Chris Seaton)]

• 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

[Eregon (Benoit Daloze)]

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.

[Dan0042 (Daniel DeLorme)]

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.

[chrisseaton (Chris Seaton)]

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.

[Dan0042 (Daniel DeLorme)]

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.

[chrisseaton (Chris Seaton)]

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

[chrisseaton (Chris Seaton)]

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