Weak ptrs for derived types?

[Mattias Nissler]

Dear friends of tasks and weak ptrs (and chromium-dev)!

Here's a question about weak ptrs vs. inheritance that I'm not sure we have a good answer for. Suppose I have the following:

     1 class Base {

     2 protected:

     3  base::WeakPtrFactory<Base> weak_factory_;

     4

     5  // Code that uses |weak_factory_|

     6 }

     7

     8 class Derived : public Base {

     9

    10  void StepOne() {

    11    MessageLoop::current()->PostTask(

    12      FROM_HERE,

    13      base::Bind(&Derived::StepTwo,

    14                 weak_factory_.GetWeakPtr());

    15  }

    16

    17  void StepTwo() {

    18  }

    19 }

This fails to compile because weak_factory_.GetWeakPtr() in line 14 will return a WeakPtr<Base>, but we need a WeakPtr<Derived>. Possible solutions I came up with:

1. Have each subclass declare their own WeakPtrFactory instance and live with the memory overhead of two (or more in case the inheritance hierarchy is deeper) WeakPtrFactory instances per class instance.
2. Cast the returned WeakPtr<Base> to a WeakPtr<Derived>. Not sure whether it's safe to assume different template instantiations to have the same memory layout?
3. Have Base inherit from SupportsWeakPtr<Base> and use base::AsWeakPtr to get a correctly-typed weak ptr. This works today, but doesn't support invalidating the weak ptrs at arbitrary times, which I need to do.
4. Introduce WeakPtrFactory::AsWeakPtr() (modeled after base::AsWeakPtr()) like this:
     template<typename Derived> WeakPtr<Derived> AsWeakPtr(Derived* ptr) {
       typedef is_convertible<Derived, T&> convertible;
       COMPILE_ASSERT(convertible::value,
                      AsWeakPtr_argument_needs_to_derive_from_base);
       CHECK_EQ(static_cast<Derived*>(ptr_), ptr);
       return WeakPtr<Derived>(weak_reference_owner_.GetRef(),
                               static_cast<Derived*>(ptr_));
     }
   
   I feel this is slightly ugly due to the fact that the factory essentially already has the pointer. I wouldn't want to introduce a type-parameterized GetWeakPtr() variant though, as that would encourage unsafe casting.     

I'm not feeling particularly happy with any of them, so I'm hoping somebody has better ideas or advice.

Thanks,

Mattias

[Gavin Peters (蓋文彼德斯)]

See base::AsWeakPtr(Derived*), defined in memory/weak_ptr.h around line 274.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Bernhard Bauer]

I think that this still requires Derived to extend SupportsWeakPtr, which Mattias didn't want to do?

Personally, I think that 1 is the way to go. A WeakPtrFactory doesn't necessarily need to vend pointers to its owner, it could vend them to any other object (i.e. in this case any other Base object), which might not be a Derived object. So it seems there is no fully type-safe way of using the base WeakPtrFactory, and then you might as well unwrap the Base object from its WeakPtr and static cast it to a Derived one, or bite the bullet and add a new WeakPtrFactory..

Bernhard.

[Darin Fisher]

We already allow conversion from WeakPtr<U> to WeakPtr<T> provided U "is a" T, so it seems reasonable to provide a down casting mechanism for WeakPtr as well.

static WeakPtr<U> WeakPtr<U>::From(const WeakPtr<T>&)

Instead, of "From", we could also go with a more explicit "DownCast" method name.  We could then delete base::AsWeakPtr in favor of this "From" method.

-Darin

 

1. Have Base inherit from SupportsWeakPtr<Base> and use base::AsWeakPtr to get a correctly-typed weak ptr. This works today, but doesn't support invalidating the weak ptrs at arbitrary times, which I need to do.
2. Introduce WeakPtrFactory::AsWeakPtr() (modeled after base::AsWeakPtr()) like this:
     template<typename Derived> WeakPtr<Derived> AsWeakPtr(Derived* ptr) {
       typedef is_convertible<Derived, T&> convertible;
       COMPILE_ASSERT(convertible::value,
                      AsWeakPtr_argument_needs_to_derive_from_base);
       CHECK_EQ(static_cast<Derived*>(ptr_), ptr);
       return WeakPtr<Derived>(weak_reference_owner_.GetRef(),
                               static_cast<Derived*>(ptr_));
     }
   
   I feel this is slightly ugly due to the fact that the factory essentially already has the pointer. I wouldn't want to introduce a type-parameterized GetWeakPtr() variant though, as that would encourage unsafe casting.     

I'm not feeling particularly happy with any of them, so I'm hoping somebody has better ideas or advice.

Thanks,

Mattias

--

[Mattias Nissler]

That would work, but it essentially allows an erroneous downcast, i.e. I could introduce

class Derived2 : public Base {}

and then do 

scoped_ptr<Derived> derived(new Derived());

WeakPtr<Base> base(derived.AsWeakPtr());

WeakPtr<Derived2> derived2(WeakPtr<Derived2>::From(base));

Do we want to encourage that? The good thing about the current base::AsWeakPtr() is that it takes a properly typed argument that determines the type of the WeakPtr it returns, so to screw up the caller needs to make an explicit erroneous downcast, i.e.:

scoped_ptr<Base> base(new Derived());

WeakPtr<Derived2> derived(base::AsWeakPtr(static_cast<Derived2*>(base.get())));

That's just for consideration though, I'm also fine with WeakPtr::From() if there's agreement that this is not considered harmful.

[Mattias Nissler]

On Mon, Jul 16, 2012 at 9:57 PM, Bernhard Bauer <bau...@google.com> wrote:

I think that this still requires Derived to extend SupportsWeakPtr, which Mattias didn't want to do?

Personally, I think that 1 is the way to go. A WeakPtrFactory doesn't necessarily need to vend pointers to its owner, it could vend them to any other object (i.e. in this case any other Base object), which might not be a Derived object. So it seems there is no fully type-safe way

I might be missing something, but base::AsWeakPtr actually gives you type-safeness by requiring to pass in a pointer of the type you want a WeakPtr of?

[Wez]

Hi Matthias,

(1) feels the least dangerous of these options, since it means that any WeakPtr<Derived> references are invalidated by the time the Derived destructor completes, meaning that any code holding such a reference and that is triggered during Base teardown won't break trying to access Derived fields.

Wez

--

[Darin Fisher]

That's a great observation!

[Albert J. Wong (王重傑)]

Interesting problem...I'd be hesitant about introducing an API that encourages downcasting.  My first reaction is to ask whether or not implementation inheritance can be avoided.  For example, you could add protected manually implemented

  virtual WeakPtr<Base> AsWeak() = 0;

and have your derived classes provide the backing WeakPtrFactory.

As to the specific suggestions below, responses inline.

That would work, but I agree that it feels less than ideal.

 

1. Cast the returned WeakPtr<Base> to a WeakPtr<Derived>. Not sure whether it's safe to assume different template instantiations to have the same memory layout?

If you mean something like:

   static_cast<WeakPtr<Derived> >(weak_factory_.GetWeakPtr());

I don't think that compiles.  There's no direct relationship between WeakPtr<Derived> and WeakPtr<Base> the templated constructor can only upcast.

1. Have Base inherit from SupportsWeakPtr<Base> and use base::AsWeakPtr to get a correctly-typed weak ptr. This works today, but doesn't support invalidating the weak ptrs at arbitrary times, which I need to do.

Hmm...we could solve this by introducing an InvalidateWeakPtrs() method into SupportsWeakPtr<>.  I'm not sure how much we want to be encouraging inheriting from SupportsWeakPtr<> over composing a WeakPtrFactory though.

1. Introduce WeakPtrFactory::AsWeakPtr() (modeled after base::AsWeakPtr()) like this:
     template<typename Derived> WeakPtr<Derived> AsWeakPtr(Derived* ptr) {
       typedef is_convertible<Derived, T&> convertible;
       COMPILE_ASSERT(convertible::value,
                      AsWeakPtr_argument_needs_to_derive_from_base);
       CHECK_EQ(static_cast<Derived*>(ptr_), ptr);
       return WeakPtr<Derived>(weak_reference_owner_.GetRef(),
                               static_cast<Derived*>(ptr_));
     }
   
   I feel this is slightly ugly due to the fact that the factory essentially already has the pointer. I wouldn't want to introduce a type-parameterized GetWeakPtr() variant though, as that would encourage unsafe casting. 

If we go the route of adding a casting API, I actually think making it type-parameterized isn't so bad.  In both APIs, the static_cast will lock you into casting within your type hierarchy and neither protect you against invalid downcasts.  In fact, in this case, I think we might as well add the unsafe cast directly in WeakPtr<> itself.

-Albert

[Jonathan Dixon]

On 16 July 2012 12:32, Mattias Nissler <mnis...@chromium.org> wrote:

Dear friends of tasks and weak ptrs (and chromium-dev)!

Here's a question about weak ptrs vs. inheritance that I'm not sure we have a good answer for. Suppose I have the following:

     1 class Base {

     2 protected:

     3  base::WeakPtrFactory<Base> weak_factory_;

     4

Slightly of off-topic, and I realize the code was for illustration only -- but note it is against style guide to ever have protected data members.

 

--

[Thiago Farina]

On Tue, Jul 17, 2012 at 2:08 AM, Jonathan Dixon <jo...@chromium.org> wrote:
>> 1 class Base {
>> 2 protected:
>> 3 base::WeakPtrFactory<Base> weak_factory_;
>> 4
>
>
> Slightly of off-topic, and I realize the code was for illustration only --
> but note it is against style guide to ever have protected data members.
>

It's against, but we deviate from that a lot (due to the lack of
careful reviews or knowledge of that recommendation?) :-(

--
Thiago

[Darin Fisher]

Some thoughts...

1-  On one hand, casting between WeakPtr<A> and WeakPtr<B> shouldn't need to be anymore restrictive than static_cast'ing between A* and B*.  WeakPtr is just a container for a raw pointer.  The only thing it adds is a way to null out the raw pointer when the object it points to disappears.

2-  The concern Wez raised is a good one.  What does it mean to have a non-null WeakPtr<Derived> pointing to a Derived object that has already executed ~Derived?  That seems like a problematic state.

#1 is what encouraged me to suggest WeakPtr<T>::From(const WeakPtr<U>&), but #2 makes me think we shouldn't support down-casting WeakPtr<Base> to WeakPtr<Derived> at all.

-Darin

--

[Mattias Nissler]

Thanks for all your input. Let me try to summarize: People feel that a generic WeakPtr casting facility could be useful, and we'd likely just add it to WeakPtr. It would perform unsafe casts and it's up to the caller to make sure their casts are valid. There's some valid concern though regarding WeakPtr<Derived> only being invalidated after the Derived dtor completes. Open question: Is that concern strong enough to abandon the idea?

On Tue, Jul 17, 2012 at 8:59 AM, Darin Fisher <da...@google.com> wrote:

Some thoughts...

1-  On one hand, casting between WeakPtr<A> and WeakPtr<B> shouldn't need to be anymore restrictive than static_cast'ing between A* and B*.  WeakPtr is just a container for a raw pointer.  The only thing it adds is a way to null out the raw pointer when the object it points to disappears.

I agree to that. My concern is that people are trained to watch out when writing static_casts, but they're probably much less careful when calling a function.

2-  The concern Wez raised is a good one.  What does it mean to have a non-null WeakPtr<Derived> pointing to a Derived object that has already executed ~Derived?  That seems like a problematic state.

Wez has a valid point. I'm not sure how much of an issue it is in practice though. A WeakPtr can only be used from one thread, so you'll only get into trouble if you have code in the dtors triggering WeakPtr accesses. IMHO that's about the same level of dragons you get with the vtable calls from dtors, and since our guideline seams to be making WeakPtrs behave as similar to raw pointers as possible, I'm not that concerned.

#1 is what encouraged me to suggest WeakPtr<T>::From(const WeakPtr<U>&), but #2 makes me think we shouldn't support down-casting WeakPtr<Base> to WeakPtr<Derived> at all.

-Darin

On Jul 16, 2012 2:25 PM, "Albert J. Wong (王重傑)" <ajw...@chromium.org> wrote:

Interesting problem...I'd be hesitant about introducing an API that encourages downcasting.  My first reaction is to ask whether or not implementation inheritance can be avoided.  For example, you could add protected manually implemented

  virtual WeakPtr<Base> AsWeak() = 0;

and have your derived classes provide the backing WeakPtrFactory.

Yes, that would suit my needs, and I'm planning to go with for now. Note that it becomes awkward in more complicated class hierarchies though as only the leaf classes should have an implementation of this function, so extending an existing leaf can introduce WeakPtrFactory duplication again.

True. It would have to be a reinterpret_cast, and I'm not going to do that :) 

[Mattias Nissler]

On Tue, Jul 17, 2012 at 11:32 AM, Mattias Nissler <mnis...@google.com> wrote:

Thanks for all your input. Let me try to summarize: People feel that a generic WeakPtr casting facility could be useful, and we'd likely just add it to WeakPtr. It would perform unsafe casts and it's up to the caller to make sure their casts are valid. There's some valid concern though regarding WeakPtr<Derived> only being invalidated after the Derived dtor completes. Open question: Is that concern strong enough to abandon the idea?

On Tue, Jul 17, 2012 at 8:59 AM, Darin Fisher <da...@google.com> wrote:

Some thoughts...

1-  On one hand, casting between WeakPtr<A> and WeakPtr<B> shouldn't need to be anymore restrictive than static_cast'ing between A* and B*.  WeakPtr is just a container for a raw pointer.  The only thing it adds is a way to null out the raw pointer when the object it points to disappears.

I agree to that. My concern is that people are trained to watch out when writing static_casts, but they're probably much less careful when calling a function.

2-  The concern Wez raised is a good one.  What does it mean to have a non-null WeakPtr<Derived> pointing to a Derived object that has already executed ~Derived?  That seems like a problematic state.

Wez has a valid point. I'm not sure how much of an issue it is in practice though. A WeakPtr can only be used from one thread, so you'll only get into trouble if you have code in the dtors triggering WeakPtr accesses. IMHO that's about the same level of dragons you get with the vtable calls from dtors, and since our guideline seams to be making WeakPtrs behave as similar to raw pointers as possible, I'm not that concerned.

#1 is what encouraged me to suggest WeakPtr<T>::From(const WeakPtr<U>&), but #2 makes me think we shouldn't support down-casting WeakPtr<Base> to WeakPtr<Derived> at all.

-Darin

On Jul 16, 2012 2:25 PM, "Albert J. Wong (王重傑)" <ajw...@chromium.org> wrote:

Interesting problem...I'd be hesitant about introducing an API that encourages downcasting.  My first reaction is to ask whether or not implementation inheritance can be avoided.  For example, you could add protected manually implemented

  virtual WeakPtr<Base> AsWeak() = 0;

and have your derived classes provide the backing WeakPtrFactory.

Yes, that would suit my needs, and I'm planning to go with for now.

Turns out I spoke to soon. In my case, I also need InvalidateWeakPtrs(), so I'd have to implement two virtuals in all derived classes (currently 3). One way around would be a type-parameterized intermediate class in the hierarchy to generate the properly-typed weak ptr operations, but that's not nice either. Back to duplicating WeakPtrFactory for now.

[Darin Fisher]

On Tue, Jul 17, 2012 at 2:32 AM, Mattias Nissler <mnis...@google.com> wrote:

Thanks for all your input. Let me try to summarize: People feel that a generic WeakPtr casting facility could be useful, and we'd likely just add it to WeakPtr. It would perform unsafe casts and it's up to the caller to make sure their casts are valid. There's some valid concern though regarding WeakPtr<Derived> only being invalidated after the Derived dtor completes. Open question: Is that concern strong enough to abandon the idea?

On Tue, Jul 17, 2012 at 8:59 AM, Darin Fisher <da...@google.com> wrote:

Some thoughts...

1-  On one hand, casting between WeakPtr<A> and WeakPtr<B> shouldn't need to be anymore restrictive than static_cast'ing between A* and B*.  WeakPtr is just a container for a raw pointer.  The only thing it adds is a way to null out the raw pointer when the object it points to disappears.

I agree to that. My concern is that people are trained to watch out when writing static_casts, but they're probably much less careful when calling a function.

2-  The concern Wez raised is a good one.  What does it mean to have a non-null WeakPtr<Derived> pointing to a Derived object that has already executed ~Derived?  That seems like a problematic state.

Wez has a valid point. I'm not sure how much of an issue it is in practice though. A WeakPtr can only be used from one thread, so you'll only get into trouble if you have code in the dtors triggering WeakPtr accesses. IMHO that's about the same level of dragons you get with the vtable calls from dtors, and since our guideline seams to be making WeakPtrs behave as similar to raw pointers as possible, I'm not that concerned.

Well, there are dragons whenever we have a Derived class inheriting from a non-trivial Base class.  If Base supports vending WeakPtrs, then perhaps we have external code holding WeakPtr<Base> objects.  I can imagine unfortunate scenarios where ~Base might generate notifications or events that might be observed by folks holding WeakPtr<Base> or worse WeakPtr<Derived>.  The dragons enter the picture because Base supports weak pointers at all!

-Darin

[Scott Hess]

On Tue, Jul 17, 2012 at 10:27 AM, Darin Fisher <da...@google.com> wrote:
> Well, there are dragons whenever we have a Derived class inheriting from a
> non-trivial Base class. If Base supports vending WeakPtrs, then perhaps we
> have external code holding WeakPtr<Base> objects.

The entire thread makes me nervous. Base is presumably returning
WeakPtr<Base> for the purpose of things which are abstracted to the
level of Base. WeakPtr<Derived> is an entirely different thing -
calling Derived-specific functions on an object returned from a Base
accessor ain't write. Just because it happens to point to the same
logical object doesn't imply that it has to use the same accessor and
weak-pointer factory.

-scott

[Darin Fisher]

nervous++

-Darin

[Wez]

Come to think of it, (2) is actually slightly worse than I thought; classes using SupportsWeakPtr derive from it, so they and their members will be torn down before any WeakPtrs get invalidated.  You need to be similarly careful to have WeakPtrFactory appear after any fields that its WeakPtrs may be used to access if you follow that pattern instead.

[Darin Fisher]

Good point.  Hmm...  maybe SupportsWeakPtr is bad.

[Jonathan Dixon]

On 17 July 2012 13:29, Darin Fisher <da...@google.com> wrote:

Good point.  Hmm...  maybe SupportsWeakPtr is bad.

IIUC It's fine for single-threaded use, which - until recently - several us thought was the only safe usage model for WeakPtr. :-)

It's a bit clunky, but we could expose a way to call weak_reference_owner_.Invaidate() to the derived class and require it to call that early on in its sub-class destructor. (the base class could even CHECK that they've done that)

[Ryan Sleevi]

On Tue, Jul 17, 2012 at 1:29 PM, Darin Fisher <da...@google.com> wrote:

Good point.  Hmm...  maybe SupportsWeakPtr is bad.

+1

In addition to being troublesome for the reasons mentioned so far, I've found several situations where the fact that SupportsWeakPtr<> exposes the threading details of the class publicly creates further problems.

For example, there are I've found several cases of situations where Base may describe a class as SupportsWeakPtr<Base>, but Derived may be RefCountedThreadSafe<Derived> (or vice versa), which really clouds the lifetime of the objects.

With an internal WeakPtrFactory (eg: weak_ptr_factory_(this), ), the only threading issues you have to particularly pay attention to are those localized to your own class, since no one can/should be able to get a WeakPtr<> directly from you, and in the worst case, you can InvalidateWeakPtrs. While not ideal, it's definitely possible to reason about, as its located to a single file.

With an external WeakPtrFactory (eg: weak_ptr_factory_(some_object_pointer_I_have)), the threading issues are again clear, in that any WeakPtr<>s vended by the factory will go away once the weak_ptr_factory_ gets dropped or Invalidate()d- which can happen regardless of the (final) lifetime of some_object_pointer_I_have.

But with SupportsWeakPtr<> + RefCountedThreadSafe<> in the same class, you can no longer safely reason about this. This is because some public caller may have asked Base/Derived for a WeakPtr<>, expecting the object to go away at a specific time, but instead find that the object is kept alive by someone else holding a reference.

http://code.google.com/p/chromium/issues/detail?id=136294 is one such example of code that was recently fixed to clear up this confusion.

I feel that while WeakPtr<>/WeakPtrFactory<> are great, SupportsWeakPtr<> is something that tends to end up leaking the threading details more than even refcounting, making it harder to reason about code. The additional corner cases related to sub-classing and destructor ordering further highlight this.