Project

General

Profile

Feature #15974

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

Added by chrisseaton (Chris Seaton) 8 months ago. Updated 7 months ago.

Status:
Closed
Priority:
Normal
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) 8 months ago

it makes sense.

Updated by mame (Yusuke Endoh) 8 months ago

Matz has approved this proposal.

Updated by shevegen (Robert A. Heiler) 8 months ago

Matz has approved this proposal.

\o/

Updated by nobu (Nobuyoshi Nakada) 8 months ago

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) 8 months ago

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.

#6

Updated by chrisseaton (Chris Seaton) 8 months ago

  • 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) 8 months ago

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) 7 months ago

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) 7 months ago

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) 7 months ago

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) 7 months ago

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

Also available in: Atom PDF