Project

General

Profile

Actions

Feature #6308

open

Eliminate delegation from WeakRef

Added by headius (Charles Nutter) almost 12 years ago. Updated about 2 years ago.

Status:
Assigned
Target version:
-
[ruby-core:<unknown>]

Description

WeakRef's delegation features are a really awful pattern that should not be allowed in future versions of Ruby.

WeakRef makes no guarantees as to the liveness of its contained object. It can be collected at any time if there are no strong references to it.

WeakRef currently uses delegation to pass method calls through to the contained object. This encourages a pattern where a WeakRef is passed to methods that expect to have a reference to the underlying object, making it appear to be that object.

Unfortunately, this is never a good idea. Because the object can be collected at any time, you may get a nil reference from getobj arbitrarily in code that tries to call methods against the given WeakRef. That means using WeakRef as a delegate will always result in unreliable code, and errors may happen for inexplicable reasons.

I believe Ruby 2.0 should eliminate WeakRef's delegation features and make it a simple reference holder. There's no safe way to use a weak reference except to grab a reference to the object, check that it is alive (non-nil) and then proceed with the use of the object, as follows:

obj = weakref.getobj
raise AppropriateError unless obj
obj.do_something
obj.do_something_else

Along with eliminating delegation, I would recommend simply making the get method #get, since the uglier #getobj is only named that way because it is not delegated.


Files

0001-weakref.rb-non-delegation.patch (1.63 KB) 0001-weakref.rb-non-delegation.patch nobu (Nobuyoshi Nakada), 10/31/2013 02:07 PM

Related issues 1 (1 open0 closed)

Related to Ruby master - Feature #6309: Add a reference queue for weak referencesAssignedmatz (Yukihiro Matsumoto)Actions

Updated by nobu (Nobuyoshi Nakada) almost 12 years ago

  • Category set to lib

It should be another new class, I think.

Updated by headius (Charles Nutter) almost 12 years ago

Perhaps under the GC module. Seems better than under ObjectSpace

I also think WeakMap should perhaps be moved under GC.

GC::WeakRef
GC::WeakMap
GC::ReferenceQueue

The top-level WeakRef library should probably be deprecated, then, and implemented in terms of GC::WeakRef.

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Status changed from Open to Feedback

Hello,

2012/4/17, headius (Charles Nutter) :

WeakRef's delegation features are a really awful pattern that should not be
allowed in future versions of Ruby.

Maybe I understood your subject, but your proposal is not clear.
Could you please make it concrete?

"change the behavior of lib/weakref.rb in 2.0"

=> Impossible, because of matz's 2.0 compatibility policy.

"change the behavior of lib/weakref.rb in 3.0"

=> Maybe possible if matz accepts. Please create a patch that warn
a user when the delegation features are used.
Then, I'll set this ticket as 3.0 issue.

"deprecate lib/weakref.rb and add an alternative library"

=> Please show us the alternative first.

--
Yusuke Endoh

Updated by headius (Charles Nutter) almost 12 years ago

My proposal is that at some time deemed acceptable by ruby-core and Matz, the delegate-based WeakRef should go away, and that in Ruby 2.0 a "preferred" non-delegate WeakRef be added (ideally along with the reference queue support in my other bug).

Here is a patch that adds a non-delegate WeakRef implementation: https://gist.github.com/2516417

This is modeled after the Java WeakRef class, which can only be traversed or cleared. Again, I hope that this would be implemented in an efficient way as part of the GC subsystem, but this implementation should show what I'm looking for. I still whole-heartedly believe that using WeakRef as a delegate is an awful, awful pattern that should be discouraged now and unavailable in the future. This patch would force users to retrieve the weak reference into a hard reference, check it for liveness, and then proceed to use the object, which is the only safe way to deal with weak references.

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Status changed from Feedback to Assigned
  • Assignee set to matz (Yukihiro Matsumoto)

Thanks, please wait a "ruling" of matz.

--
Yusuke Endoh

Updated by headius (Charles Nutter) over 11 years ago

Seven months and no activity. I still would like to see delegate-based Weakref go away, but I know the official plan is to have no backward compatibility. Unfortunately, delegate-based Weakref is still a bad, broken implementation, and I think it should go away. 2.0 is as good a time as any.

Updated by mame (Yusuke Endoh) over 11 years ago

  • Target version set to 2.6

Updated by headius (Charles Nutter) about 11 years ago

I request a ruling by matz for making the backward-incompatible change of having Weakref no longer be a Delegate. Alternatively, make it a Delegate where all delegated methods warn that you shouldn't use it as a Delegate.

If necessary I will move this feature request to CommonRuby.

Actions #9

Updated by headius (Charles Nutter) almost 11 years ago

  • Project changed from Ruby master to 14
  • Category deleted (lib)
  • Target version deleted (2.6)

Moving into CommonRuby

Updated by headius (Charles Nutter) almost 11 years ago

Here's a patch that removes delegation from Weakref: https://github.com/headius/ruby/commit/431b971a147daf8a9b2185d117580a842f3c8dfc

One test was no longer really relevant (and not adding anything), and others were modified to reflect this change.

Updated by headius (Charles Nutter) over 10 years ago

I believe this is just waiting on approval by matz. Can we make this change in 2.1, please?

Updated by headius (Charles Nutter) over 10 years ago

  • Target version set to Ruby 2.1.0

Updated by headius (Charles Nutter) over 10 years ago

Put my patch into an updated pull request: https://github.com/ruby/ruby/pull/406

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

My opinion is still unchanged.
"It should be another new class".

Updated by headius (Charles Nutter) over 10 years ago

nobu (Nobuyoshi Nakada) wrote:

My opinion is still unchanged.
"It should be another new class".

In case my opinion wasn't clear, or has been fogged over the last few months, I'll make it clear again.

WeakRef is broken when used as a delegate, because any method at any time might raise an error the code is not prepared to handle. This is filed as a feature, but it's actually a bug fix to eliminate the buggy behavior of transparently delegating method calls through WeakRef to its object.

Introduce another class called what? UndelegatedWeakRef? WeakRefThatIsNotBroken? UseThisWeakRefBecauseTheOtherOneWillRaiseErrorsRandomly? :-)

Introducing another class will just add confusion to stdlib and allow the current broken WeakRef to perpetuate. We did not want any breaking changes in 2.0, but I think we really need to do this in 2.1.

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

headius (Charles Nutter) wrote:

Introducing another class will just add confusion to stdlib and allow the current broken WeakRef to perpetuate. We did not want any breaking changes in 2.0, but I think we really need to do this in 2.1.

Changing existing class drastically breaks existing code all.
It's not acceptable.

For new name, what about WeakRef.[]?

Updated by nobu (Nobuyoshi Nakada) over 10 years ago

Chained WeakRef (WeakRef to WeakRef) doesn't work well without delegation.

w = WeakRef.new(obj)
w = WeakRef.new(w)
w.foo

and

w = WeakRefWithoutDelegation.new(obj)
w = WeakRefWithoutDelegation.new(w)
w.get.get.foo

How do you think?

Updated by headius (Charles Nutter) over 10 years ago

nobu (Nobuyoshi Nakada) wrote:

Chained WeakRef (WeakRef to WeakRef) doesn't work well without delegation.

w = WeakRef.new(obj)
w = WeakRef.new(w)
w.foo

I can't say I've ever had a need to wrap a weakref in a weakref. What's the use case?

and

w = WeakRefWithoutDelegation.new(obj)
w = WeakRefWithoutDelegation.new(w)
w.get.get.foo

How do you think?

This looks fine to me. WeakRef is a reference holder, so calling get is correct.

Let me summarize reasons why delegation is bad:

  • Calls can fail at any time.
  • Error handling is not possible because other consumers of the WeakRef won't know it's a WeakRef.
  • On earlier versions of Ruby (pre-2.0) another object could get the same object_id and you'd call against the wrong object.

The bottom line is that delegating through a WeakRef is a big potential bug if you're not constantly checking that the referenced object still exists.

nobu (Nobuyoshi Nakada) wrote:

Changing existing class drastically breaks existing code all.
It's not acceptable.

Many behaviors change in breaking ways in x.y releases. I contend that WeakRef transparently delegating is a bug that needs to be fixed.

For new name, what about WeakRef.[]?

I'll say it one more time: I don't believe anyone should be using the existing WeakRef behavior. Adding another class won't fix code that's out there doing things wrong (and delegating through WeakRef is wrong).

If we have to introduce a new class (I strongly protest) something like SimpleWeakRef might be better than a symbolic name.

Updated by headius (Charles Nutter) over 10 years ago

headius (Charles Nutter) wrote:

If we have to introduce a new class (I strongly protest) something like SimpleWeakRef might be better than a symbolic name.

Or how about WeakReference, and we add a warning saying WeakRef is deprecated? I can accept that.

Updated by headius (Charles Nutter) over 10 years ago

I have updated my PR to put the new non-delegating weak reference in a WeakReference class and add a deprecation warning to WeakRef.

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

Actions #21

Updated by hsbt (Hiroshi SHIBATA) about 2 years ago

  • Project changed from 14 to Ruby master
  • Target version deleted (Ruby 2.1.0)
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0