PSA: Delaying destruction is generally the wrong solution

[William Chan (陈智昌)]

TL;DR: Don't delay destruction (PostTask or refcounting), fix your destruction order / lifetime semantics.

Let's say you have A which owns B which owns C. C accesses A via a Delegate interface, which is the Chromium paradigm for calling back up the stack without violating layering. In a normal sequence of events, A's destruction would trigger B's destruction, which would trigger C's destruction, which would prevent C from calling back into A. Now, let's say A is destroyed, which *should* trigger B's destruction, but to fix another bug, you delay B's destruction. This also delays the destruction of all the objects that B transitively owns. These objects now will not get the destruction / shutdown notification that would prevent them from calling back up the stack via a Delegate interface. Now you have a use-after-free (C calling into the already freed A) that basically no one understands and it requires engineers who understand the relationships across A, B, C, ... Z to spend many engineering hours to figure out what is causing the use-after-free bug.

I understand why these destruction delay bandaids are initially alluring, since they fix the problem, but lead to much more subtle ones in different subsystems that aren't initially caught in normal testing. Also, fixing destruction ordering / lifetime semantics is difficult because it may require understanding lifetime semantics across different subsystems. Remember though, if you don't understand these semantics, you are forcing incurring significant costs on the stability and security teams who get these mysterious use-after-frees without obvious root causes, and have to spend time hunting down the culprit. Also remember that, even if C does not access A via a Delegate interface today, it's a very natural thing to happen in the future, and if the author of the Delegate doesn't see any problems during testing, then it'll result in mysterious crash reports that are no fun to debug.

I mentioned refcounting, because it's a variant of delaying destruction. Let's say X owns Y, and it turns out the Z references Y (but Z doesn't really own Y, which is why Y isn't already refcounted). The alluring option is to simply refcount Y, despite the fact that Z doesn't own Y. Z was supposed to be have been destroyed at the correct time during shutdown, but that didn't happen. So we refcount Y, and now X gets deleted, but Y does not, due to Z's destruction ordering being broken, so now objects that Y owns that call into X via a Delegate interface or something cause use-after-frees. Bad news. I hate refcounting.

So please, stop delaying destruction. Spend the time to figure out why the destruction order isn't occurring as expected, and fix it.

PS: I'm off on vacation for a few weeks, so I probably won't respond :) So flame away!

[Peter Kasting]

Just as a general tip, in many cases where you might use refcounting to allow an object to be safely used on multiple threads, you can often find a way to use WeakPtrFactory instead, which is almost always safer and easier to reason about.

PK

[Wez]

+1

On 29 June 2012 18:08, Peter Kasting <pkas...@chromium.org> wrote:

Just as a general tip, in many cases where you might use refcounting to allow an object to be safely used on multiple threads, you can often find a way to use WeakPtrFactory instead, which is almost always safer and easier to reason about.

PK

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

[Szymon Jakubczak]

+1 to WeakPtr. I hate the feeling of zombies roaming around looking to use-after-free things, because some reference is keeping it alive.

[Carlos Pizano]

I don't see anything controversial in William's observation. I say that somebody with the gift of English should edit  http://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures  because as-is the point very weakly made and you don't get the feeling that WeakPtrFactory is a good solution, even better a section about delegates.

[Elliot Glaysher (Chromium)]

+1

[Lambros Lambrou]

Unfortunately, that document (http://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures) describes WeakPtr as "mostly thread-unsafe", and that "it should only be used on the original thread it was created on".  Also, the header file says "When you get a WeakPtr ... the WeakPtr becomes bound to the current thread."  And, the fatal blow: "You may only dereference the WeakPtr on that thread".  Ouch ouch ouch!

I don't know if I'm typical of most engineers here, but these statements are enough to scare me off using WeakPtr altogether, in favor of using RefCountedThreadSafe (which feels impossible to use wrongly, and therefore much safer).

If I can't "use" (whatever that means) a WeakPtr on a different thread, surely that makes the class almost useless?  What am I missing?

If you are editing that document, and you understand WeakPtr, please explain when it is safe to use, and why it's safe in those circumstances.  Otherwise, I'm simply too scared to use it at all! :)

Lambros

[Peter Kasting]

One obviously-safe technique is to ensure that all accesses to your object happen on the same thread, even if callers might normally live on different threads.  Then you can vend weak pointers to all the callers and they can rely on PostTask()/PostTaskAndReply()/etc. auto-cancelling once the weak pointers are NULLed.  This is an appealing route anyway because it means your object doesn't need locks to protect members that get accessed across threads, which makes your class simpler, safer, and more performant.

PK

[Ryan Sleevi]

On Mon, Jul 2, 2012 at 10:33 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

Unfortunately, that document (http://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures) describes WeakPtr as "mostly thread-unsafe", and that "it should only be used on the original thread it was created on".  Also, the header file says "When you get a WeakPtr ... the WeakPtr becomes bound to the current thread."  And, the fatal blow: "You may only dereference the WeakPtr on that thread".  Ouch ouch ouch!

I don't know if I'm typical of most engineers here, but these statements are enough to scare me off using WeakPtr altogether, in favor of using RefCountedThreadSafe (which feels impossible to use wrongly, and therefore much safer).

If I can't "use" (whatever that means) a WeakPtr on a different thread, surely that makes the class almost useless?  What am I missing?

If you are editing that document, and you understand WeakPtr, please explain when it is safe to use, and why it's safe in those circumstances.  Otherwise, I'm simply too scared to use it at all! :)

Lambros

With WeakPtr, they can be copy-constructed and destructed on arbitrary threads. They're thread-safe in that regard, as they're necessary for base::Bind/base::Callback semantics. What they're not thread-safe about is de-referencing, which the docs specifically call out.

Largely, it means that this pattern is UNSAFE

// Called on file thread

void MyClass::DoSomethingOnFileThread() {

  // Do some work

  int result = DoSomeWork();

  // NOT SAFE.

  io_thread_->PostTask(FROM_HERE, base::Bind(&MyClass::DoSomethingOnIOThread, weak_ptr_factory_.GetWeakPtr(), result);

}

This is because the returned WeakPtr is created on the FILE thread, but will be dereferenced on the IO thread.

However, the encouragement for WeakPtr is that it can be used to decouple the lifetimes between callers, particularly when used with PostTaskAndReply/PostTaskAndReplyWithResult.

// Called on FILE thread

void ClassA::DoSomething() {

  class_b->DoSomethingElse(base::Bind(&ClassA::HandleResults, weak_ptr_factory_.GetWeakPtr()));

}

// Called on ANY thread.

// |callback| will be invoked on the same thread this is called from. In this case, the FILE thread.

void ClassB::DoSomethingElse(const base::Callback<int>& calback) {

  PostTaskAndReplyWithHelper(io_thread_, FROM_HERE, base::Bind(&ClassB::DoSomethingOnIOThread, base::Unretained(this)), callback);

}

// Called on IO thread

int ClassB::DoSomethingOnIOThread() {

  int rv = DoSomeWork();

  return rv;

}

With such an approach, ClassA can disappear at /any/ time - even while ClassB is off busy doing work on the IO thread. The only requirements here are that ClassB remains valid for as long as a ClassA is around - aka, your normal threading/ownership guarantees.

When ClassB::DoSomethingOnIOThread completes, the response will be posted back to the FILE thread. If ClassA has disappeared, the callback will no-op. With a RefCountedThreadSafe approach, the reference to ClassA will be kept alive until ClassB completes - and further, ClassA::HandleResults will be invoked, which means you need to guarantee any objects needed by HandleResults will also be alive.

Part of the WeakPtr approach is trying to encourage a design where each object has a single thread it lives on - both birth and death. While it may be thread-safe (callable from multiple threads), the lifetime itself has strong guarantees. For classes that are refcounted and may live on multiple threads, this often requires splitting up the class into distinct parts. That is, here's the part that lives on the FILE thread, here's the part that lives on the IO thread, and then wrapped by a super-class to manage ownership of these helpers and provide the thread-safe interface.

That is, WeakPtr is specifically trying to DISCOURAGE this pattern:

// Not ideal. Ambiguous lifetime for ClassA.

class ClassA : public base::RefCountedThreadSafe<ClassA> {

  ...

};

// Called on IO thread.

void ClassA::CalledOnIOThread() {

  file_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::CalledOnFileThread, this));

}

// Called on FILE thread

void ClassA::CalledOnFileThread() {

  int result = DoWork();

  io_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::HandleResults, this, result));

}

// Called on IO thread.

void ClassA::HandleResults(int result) {

  // ...

[Lambros Lambrou]

Ah, that makes sense!  So, by "using" a WeakPtr, we mean either "dereferencing it to call a method on it", or "dereferencing it to destroy the object pointed to" (which is the same thing, really).

In the case where the WeakPtr is unsafe to use (deferencing it in two different threads), then the operation is inherently unsafe anyway on the underlying object (without locks).  Gotcha!  So the WeakPtr interface isn't introducing any extra danger in that case.

It does mean, however, that if you have an object that might be destroyed on a different thread (and you have locks to deal with that if necessary), the WeakPtr class doesn't help.  Of course, the better solution is to have the would-be-destroyer post a task to the proper thread, instead of destroying it directly.

I'm now guessing that the reason we have WeakPtrFactory is so we can ensure that the WeakPtr gets created on the correct thread.  So the shortcoming of the WeakPtr interface (the fact that it's thread-unsafe) is addressed by the WeakPtrFactory class.  Have I got this right?

Thanks for helping me understand this.  I still think the documentation could be improved to make others less scared of using this class.  I will try to digest Ryan's reply as well, before tackling this further.

Lambros

[Peter Kasting]

On Mon, Jul 2, 2012 at 11:42 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

Ah, that makes sense!  So, by "using" a WeakPtr, we mean either "dereferencing it to call a method on it", or "dereferencing it to destroy the object pointed to" (which is the same thing, really).

"Use" means any actual access, I believe, including checking whether the pointer has been NULLed.  (Someone correct me if I'm wrong.)

In the case where the WeakPtr is unsafe to use (deferencing it in two different threads), then the operation is inherently unsafe anyway on the underlying object (without locks).  Gotcha!  So the WeakPtr interface isn't introducing any extra danger in that case.

To be even more specific: "inherently unsafe... without locks and lifetime guarantees".  In the context of this thread, a pattern we're trying to avoid is using threadsafe refcounting to ensure lifetimes, but yes, as noted here, that still doesn't ensure you can actually safely touch a particular object on multiple threads, just that it won't have been destroyed.

It does mean, however, that if you have an object that might be destroyed on a different thread (and you have locks to deal with that if necessary), the WeakPtr class doesn't help.  Of course, the better solution is to have the would-be-destroyer post a task to the proper thread, instead of destroying it directly.

Right.

I'm now guessing that the reason we have WeakPtrFactory is so we can ensure that the WeakPtr gets created on the correct thread.  So the shortcoming of the WeakPtr interface (the fact that it's thread-unsafe) is addressed by the WeakPtrFactory class.  Have I got this right?

I don't recall the precise semantics of WeakPtrFactory on multiple threads.

As Ryan spoke about in more detail, the design pattern we're really trying to push here is for individual objects to be created, accessed, and destroyed all on the same thread.  The combination of Bind() (and the PostTask(), etc. framework around it) and WeakPtr makes it very easy to build distinct objects, which live on different threads, that can safely talk to each other without leaks, refcounting, or complex manual signalling via callbacks.  And ensuring that each individual object lives only on one thread eliminates locks and simplifies the mental model a maintainer has to understand.

(Plus, threadsafe refcounting is much more expensive than non-threadsafe refcounting, so even if ultimately you end up needing to refcount for some reason, at least not having that refcounting happen across multiple threads is faster and generally much easier to understand.  But most of the time you can avoid even that.)

PK

[Jeffrey Yasskin]

On Mon, Jul 2, 2012 at 12:01 PM, Peter Kasting <pkas...@chromium.org> wrote:
> (Plus, threadsafe refcounting is much more expensive than non-threadsafe
> refcounting, so even if ultimately you end up needing to refcount for some
> reason, at least not having that refcounting happen across multiple threads
> is faster and generally much easier to understand. But most of the time you
> can avoid even that.)

A nit: while a thread-safe refcount operation is something like 50x as
expensive as a thread-unsafe refcount operation, it's still cheaper
than a single L2 cache miss (which, admittedly, may be caused by
cross-thread refcounting among many other things), and Will mentioned
that he switched all refcounts in Chrome to thread-safe without seeing
a performance change. (Unfortunately, I don't have a link to his
numbers.)

Jeffrey

[Jonathan Dixon]

On 2 July 2012 11:42, Lambros Lambrou <lambros...@chromium.org> wrote:

Ah, that makes sense!  So, by "using" a WeakPtr, we mean either "dereferencing it to call a method on it", or "dereferencing it to destroy the object pointed to" (which is the same thing, really).

In the case where the WeakPtr is unsafe to use (deferencing it in two different threads), then the operation is inherently unsafe anyway on the underlying object (without locks).  Gotcha!  So the WeakPtr interface isn't introducing any extra danger in that case.

It does mean, however, that if you have an object that might be destroyed on a different thread (and you have locks to deal with that if necessary), the WeakPtr class doesn't help.  Of course, the better solution is to have the would-be-destroyer post a task to the proper thread, instead of destroying it directly.

I'm now guessing that the reason we have WeakPtrFactory is so we can ensure that the WeakPtr gets created on the correct thread.  So the shortcoming of the WeakPtr interface (the fact that it's thread-unsafe) is addressed by the WeakPtrFactory class.  Have I got this right?

Thanks for helping me understand this.  I still think the documentation could be improved to make others less scared of using this class.  I will try to digest Ryan's reply as well, before tackling this further.

For the record, back when I was learning the chromium code base, I was also steered away from using WeakPtr due to the same stern thread safety warnings in that file. (also back then weakptr was brand new and unfamiliar to my reviewers, whereas RefCounted was well used and understood).

So +1 to adding some words to the effect that 'weak ptr is fine to pass from thread to thread, e.g. as a request owner's unique token, so long as the eventually usage of the weak ptr ["dereference"] only occurs on the thread on which it originated.'

 

Lambros

On Mon, Jul 2, 2012 at 10:46 AM, Peter Kasting <pkas...@chromium.org> wrote:

One obviously-safe technique is to ensure that all accesses to your object happen on the same thread, even if callers might normally live on different threads.  Then you can vend weak pointers to all the callers and they can rely on PostTask()/PostTaskAndReply()/etc. auto-cancelling once the weak pointers are NULLed.  This is an appealing route anyway because it means your object doesn't need locks to protect members that get accessed across threads, which makes your class simpler, safer, and more performant.

PK

[Wez]

+1 to improving the WeakPtr wording.

The current description talks about "using" the WeakPtr on the thread on which it was created. Firstly, as Lambros remarked, the term "use" is ambiguous and what's really meant is that it must only be tested or dereferenced on that thread. Secondly, the important threading constraint is that WeakPtrs should only ever be dereferenced on the thread on which the object they refer to will be deleted.

[Peter Kasting]

On Mon, Jul 2, 2012 at 2:21 PM, Wez <w...@chromium.org> wrote:

Secondly, the important threading constraint is that WeakPtrs should only ever be dereferenced on the thread on which the object they refer to will be deleted.

Wait, is that right?  Or is it that a WeakPtr must only by accessed on the thread on which the WeakPtr was created?

(I realize that in the scenario I outlined, where an object is only accessed on one thread, and WeakPtrs are all vended on that thread, the distinction is immaterial.)

PK

[Wez]

The documentation is as you describe, Peter, but if you go dereferencing a WeakPtr, and using the object, on a thread other than the one on which the object is deleted then either you already have some teardown ordering guarantee in place that means you don't need to use a WeakPtr, or you're introducing a fun race condition.

[Ryan Sleevi]

The WeakPtr must only be accessed on the thread on which the WeakPtr was created. Generally, this will/should be the same thread on which the object-pointed-to will also be deleted.

The distinction is that, with a WeakPtrFactory does not necessarily have to be a member of the object-being-pointed-to - it COULD be that ClassA owns-a ClassB* (class_b_) and ClassA has-a WeakPtrFactory<ClassB> (weak_ptr_factory_ , initialized with WeakPtrFactory(class_b_) )

This scenario permits ClassA calling weak_ptr_factory_.InvalidateWeakPtrs() on the thread that vends WeakPtrs (Thread 2), but deleting ClassB on a different thread (Thread 1).

While mostly academic/pedantic, the above scenario provides a way to safely 'stop' all pending calls on an object, regardless of how many Tasks/Callbacks are pending. You still have to worry about synchronizing the two threads - eg: before deleting ClassB on Thread 1, you need to make sure that no ClassB methods are currently running on Thread 2. InvalidateWeakPtrs will simply prevent /new/ methods from being called.

Mostly, the above makes it easy to use WeakPtr as a way to implement "cancellable" callbacks without requiring explicit cancellation tracking in the Task/Callback or requiring wrapper objects.

[Wez]

That's true, but the constraint still isn't that the WeakPtr must be used on the thread on which it's created, but that it be used on the thread on which the object which it will be invalidated (whether implicitly via SupportsWeakPtr deletion, or via a WeakPtrFactory).  In your example it could be created on Thread 1, in principle, so long as it's only ever dereferenced on Thread 2, where it will be invalidated.

[David Michael]

On Mon, Jul 2, 2012 at 4:16 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 3:04 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 2:21 PM, Wez <w...@chromium.org> wrote:

Secondly, the important threading constraint is that WeakPtrs should only ever be dereferenced on the thread on which the object they refer to will be deleted.

Wait, is that right?  Or is it that a WeakPtr must only by accessed on the thread on which the WeakPtr was created?

(I realize that in the scenario I outlined, where an object is only accessed on one thread, and WeakPtrs are all vended on that thread, the distinction is immaterial.)

PK

The WeakPtr must only be accessed on the thread on which the WeakPtr was created.

This is enforced by a ThreadChecker, so definitely true in practice.

 

Generally, this will/should be the same thread on which the object-pointed-to will also be deleted.

Better to do all of these on 1 thread. But if it wasn't for the use of ThreadChecker, the thread it's deleted on would matter more. You don't want a race where the pointer appears to be valid just before you use it.

The distinction is that, with a WeakPtrFactory does not necessarily have to be a member of the object-being-pointed-to - it COULD be that ClassA owns-a ClassB* (class_b_) and ClassA has-a WeakPtrFactory<ClassB> (weak_ptr_factory_ , initialized with WeakPtrFactory(class_b_) )

I would advise against doing the above. SupportsWeakPtr is a safer way. It's critical that the lifetime of the WeakPtrFactory is scoped to the lifetime of its pointee, or all bets are off. If you separate the two, you're opening yourself up to make a mistake about their order of destruction.

[Ryan Sleevi]

On Mon, Jul 2, 2012 at 3:49 PM, David Michael <dmic...@google.com> wrote:

On Mon, Jul 2, 2012 at 4:16 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 3:04 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 2:21 PM, Wez <w...@chromium.org> wrote:

Secondly, the important threading constraint is that WeakPtrs should only ever be dereferenced on the thread on which the object they refer to will be deleted.

Wait, is that right?  Or is it that a WeakPtr must only by accessed on the thread on which the WeakPtr was created?

(I realize that in the scenario I outlined, where an object is only accessed on one thread, and WeakPtrs are all vended on that thread, the distinction is immaterial.)

PK

The WeakPtr must only be accessed on the thread on which the WeakPtr was created.

This is enforced by a ThreadChecker, so definitely true in practice.

 

Generally, this will/should be the same thread on which the object-pointed-to will also be deleted.

Better to do all of these on 1 thread. But if it wasn't for the use of ThreadChecker, the thread it's deleted on would matter more. You don't want a race where the pointer appears to be valid just before you use it.

The distinction is that, with a WeakPtrFactory does not necessarily have to be a member of the object-being-pointed-to - it COULD be that ClassA owns-a ClassB* (class_b_) and ClassA has-a WeakPtrFactory<ClassB> (weak_ptr_factory_ , initialized with WeakPtrFactory(class_b_) )

I would advise against doing the above. SupportsWeakPtr is a safer way. It's critical that the lifetime of the WeakPtrFactory is scoped to the lifetime of its pointee, or all bets are off. If you separate the two, you're opening yourself up to make a mistake about their order of destruction.

No, it is not at all critical that these two be related. Providing this disconnect is one of the key points of InvalidateWeakPtrs.

Since InvalidateWeakPtrs can only be called on the thread that the WeakPtrs run/will be dereferenced, you have a 100% guarantee that "some other thread" will not be in the pointee's code - because they're in your code. So it is totally valid (and totally safe) to do something like:

// Runs on IO thread

void ClassA::CalledOnIOThread() {

  file_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::CleanupOnFileThread, base::Unretrained(this));

}

// Runs on FILE thread

void ClassA::CleanupOnFileThread() {

  // weak_ptr_factory_ holds-a ClassB

  // weak_ptr_factory_ and ClassB are only ever used on the FILE thread

  weak_ptr_factory_->InvalidateWeakPtrs();

  // Not strictly necessary - but this pattern here is a sanity check and reflects the general problem with

  // objects living on multiple threads. The assumption here is that some other method on ClassA may be

  // pending on the FILE thread that will attempt to vend a WeakPtr.

  weak_ptr_factory_.reset();

  io_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::CleanupComplete, base::Unretained(this));

}

// Called on the IO thread

void ClassA::CleanupComplete() {

  // Totally safe to do. All of the WeakPtrs have been invalidated, so nobody on the FILE thread can or

  // will be using |class_b_| at this point.

  class_b_.release();

}

The discussion here is the normal discussion of inheritance vs composition, and is similar of the discussion of external reference counting vs intrusive reference counting. In Chromium code, the style guide strongly encourages composition, for which the above is totally safe to do.

SupportsWeakPtr<> can be as dangerous as reference counting, and requires care and forethought. Using SupportsWeakPtr<> publicly exposes the ability to vend WeakPtrs. This can lead to callers having a Foo* and trying to vend a WeakPtr<Foo> from it - which they may be on the wrong thread and this may not be safe to do!

In many cases, if your use across multiple threads is an implementation detail (eg: you provide yourself as a thread-safe interface), then you should not inherit from SupportsWeakPtr, and instead should have a WeakPtrFactory yourself. This ensures no one else can misappropriately vend a WeakPtr from your Class* pointer.

This also reflects Peter's point (and something Will has raised multiple times), that it's often better and easier to reason about when objects to live on a single thread. In that case, you don't need to expose any WeakPtr semantics at all - and instead, your true "owner" should be using it - either by using a WeakPtr to guard themselves and a scoped_ptr<> to guard you, or using a WeakPtrFactory<> to guard you and using some other semantics.

[John Knottenbelt]

I'm finding this discussion really helpful. It would be really awesome to have the patterns of concurrency that we prefer to use in Chromium exemplified on the developer site, perhaps linked to via http://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures .

I agree that the concept of "individual objects to be created, accessed, and destroyed" simplifies reasoning about concurrency issues enormously. With regard to this, is it acceptable for an object that lives/dies on one thread to own an object that lives/dies on another thread? Is there a canonical way of expressing such an ownership relationship? scoped_ptr and scoped_vector seem to be appropriate only if both objects live on the same thread. 

[Dominic Battre]

On Mon, Jul 2, 2012 at 8:35 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 10:33 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

Unfortunately, that document (http://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures) describes WeakPtr as "mostly thread-unsafe", and that "it should only be used on the original thread it was created on".  Also, the header file says "When you get a WeakPtr ... the WeakPtr becomes bound to the current thread."  And, the fatal blow: "You may only dereference the WeakPtr on that thread".  Ouch ouch ouch!

I don't know if I'm typical of most engineers here, but these statements are enough to scare me off using WeakPtr altogether, in favor of using RefCountedThreadSafe (which feels impossible to use wrongly, and therefore much safer).

If I can't "use" (whatever that means) a WeakPtr on a different thread, surely that makes the class almost useless?  What am I missing?

If you are editing that document, and you understand WeakPtr, please explain when it is safe to use, and why it's safe in those circumstances.  Otherwise, I'm simply too scared to use it at all! :)

Lambros

With WeakPtr, they can be copy-constructed and destructed on arbitrary threads. They're thread-safe in that regard, as they're necessary for base::Bind/base::Callback semantics. What they're not thread-safe about is de-referencing, which the docs specifically call out.

Largely, it means that this pattern is UNSAFE

// Called on file thread

void MyClass::DoSomethingOnFileThread() {

  // Do some work

  int result = DoSomeWork();

  // NOT SAFE.

  io_thread_->PostTask(FROM_HERE, base::Bind(&MyClass::DoSomethingOnIOThread, weak_ptr_factory_.GetWeakPtr(), result);

}

This is because the returned WeakPtr is created on the FILE thread, but will be dereferenced on the IO thread.

However, the encouragement for WeakPtr is that it can be used to decouple the lifetimes between callers, particularly when used with PostTaskAndReply/PostTaskAndReplyWithResult.

// Called on FILE thread

void ClassA::DoSomething() {

  class_b->DoSomethingElse(base::Bind(&ClassA::HandleResults, weak_ptr_factory_.GetWeakPtr()));

}

// Called on ANY thread.

// |callback| will be invoked on the same thread this is called from. In this case, the FILE thread.

void ClassB::DoSomethingElse(const base::Callback<int>& calback) {

  PostTaskAndReplyWithHelper(io_thread_, FROM_HERE, base::Bind(&ClassB::DoSomethingOnIOThread, base::Unretained(this)), callback);

}

Can you remind me why this call to base::Unretained(this) is safe? If the object of ClassB is deallocated while the task is in the queue, this will fail, right?

Or more general: What is the desired pattern to call yourself on another thread?

Best regards,

Dominci

[David Michael]

On Mon, Jul 2, 2012 at 5:50 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 3:49 PM, David Michael <dmic...@google.com> wrote:

On Mon, Jul 2, 2012 at 4:16 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 3:04 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Jul 2, 2012 at 2:21 PM, Wez <w...@chromium.org> wrote:

Secondly, the important threading constraint is that WeakPtrs should only ever be dereferenced on the thread on which the object they refer to will be deleted.

Wait, is that right?  Or is it that a WeakPtr must only by accessed on the thread on which the WeakPtr was created?

(I realize that in the scenario I outlined, where an object is only accessed on one thread, and WeakPtrs are all vended on that thread, the distinction is immaterial.)

PK

The WeakPtr must only be accessed on the thread on which the WeakPtr was created.

This is enforced by a ThreadChecker, so definitely true in practice.

 

Generally, this will/should be the same thread on which the object-pointed-to will also be deleted.

Better to do all of these on 1 thread. But if it wasn't for the use of ThreadChecker, the thread it's deleted on would matter more. You don't want a race where the pointer appears to be valid just before you use it.

The distinction is that, with a WeakPtrFactory does not necessarily have to be a member of the object-being-pointed-to - it COULD be that ClassA owns-a ClassB* (class_b_) and ClassA has-a WeakPtrFactory<ClassB> (weak_ptr_factory_ , initialized with WeakPtrFactory(class_b_) )

I would advise against doing the above. SupportsWeakPtr is a safer way. It's critical that the lifetime of the WeakPtrFactory is scoped to the lifetime of its pointee, or all bets are off. If you separate the two, you're opening yourself up to make a mistake about their order of destruction.

No, it is not at all critical that these two be related. Providing this disconnect is one of the key points of InvalidateWeakPtrs.

You might be misunderstanding what I'm saying. I agree, it's fine to invalidate weak pointers any time prior to the destruction of the pointee. I'm pointing out that the WeakPtrFactory's pointee must remain valid during the life of the WeakPtrFactory. The usual pattern accomplishes this very neatly, because the WeakPtrFactory points to its owner, and therefore must be destroyed before its pointee. In the absence of that pattern, you have to be careful to provide the guarantee. Your describing something like this:

class ClassA {

  ClassB b_;

  WeakPtrFactory<ClassB> b_ptr_factory_;

};

If instead, you did:

class ClassA {

  WeakPtrFactory<ClassB> b_ptr_factory_;

  ClassB b_;

};

(or other possible screwups if you own the ClassB by pointer)... then b_ will be destroyed before b_ptr_factory_, and b_ptr_factory_ has no clue about it. So if you try to dereference a WeakPtr from b_ptr_factory_ in that window of time, it's a use-after-free. (Admittedly In this particular case, there's only a very small window for making a mistake.)

 

Since InvalidateWeakPtrs can only be called on the thread that the WeakPtrs run/will be dereferenced, you have a 100% guarantee that "some other thread" will not be in the pointee's code - because they're in your code. So it is totally valid (and totally safe) to do something like:

// Runs on IO thread

void ClassA::CalledOnIOThread() {

  file_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::CleanupOnFileThread, base::Unretrained(this));

}

// Runs on FILE thread

void ClassA::CleanupOnFileThread() {

  // weak_ptr_factory_ holds-a ClassB

  // weak_ptr_factory_ and ClassB are only ever used on the FILE thread

  weak_ptr_factory_->InvalidateWeakPtrs();

  // Not strictly necessary - but this pattern here is a sanity check and reflects the general problem with

  // objects living on multiple threads. The assumption here is that some other method on ClassA may be

  // pending on the FILE thread that will attempt to vend a WeakPtr.

  weak_ptr_factory_.reset();

  io_thread_->PostTask(FROM_HERE, base::Bind(&ClassA::CleanupComplete, base::Unretained(this));

}

// Called on the IO thread

void ClassA::CleanupComplete() {

  // Totally safe to do. All of the WeakPtrs have been invalidated, so nobody on the FILE thread can or

  // will be using |class_b_| at this point.

  class_b_.release();

}

The discussion here is the normal discussion of inheritance vs composition, and is similar of the discussion of external reference counting vs intrusive reference counting. In Chromium code, the style guide strongly encourages composition, for which the above is totally safe to do.

SupportsWeakPtr<> can be as dangerous as reference counting, and requires care and forethought. Using SupportsWeakPtr<> publicly exposes the ability to vend WeakPtrs. This can lead to callers having a Foo* and trying to vend a WeakPtr<Foo> from it - which they may be on the wrong thread and this may not be safe to do!

I agree, SupportsWeakPtr<> has its own pitfalls.

[Ryan Sleevi]

In the above example, the concept being demonstrated was that ClassA can safely disappear while ClassB is still working and still has a callback that may eventually try to call ClassA (but won't, due to the WeakPtr). The lifetime of ClassB wasn't really demonstrated. Minimally, it would be presumed that it must outlive all classes that a naked pointer has been given to (eg: it must outlive ClassA).

Further, since ClassB seems (from the small example) to be doing all its work on the IO thread, it would likely be argued that it "lives" on the IO thread, and thus should be shut down / destroyed on that thread - such as using DeleteSoon, either by itself or by the object that truly owns the ClassB*.

If ClassB were destroyed on the FILE thread, while tasks were pending/executing on the IO thread, yes, this would be problematic.

 

Or more general: What is the desired pattern to call yourself on another thread?

The very-high-level answer is "In general, don't". That's the general preference for using objects that are single-threaded.

However, in practice, this isn't always reasonable.

- Does your interface have specific requirements on destruction ordering? Such as "There can be no pending operations" (eg: something you can DCHECK in the destructor)? If so, you can use these guarantees to know if you have messages pending that are posted to yourself.

- Does your interface provide a thread-safe public-interface? Do you require all callers call you on a particular thread? Then having a WeakPtrFactory member and posting a WeakPtr between yourself may be appropriate.

- Do you have a more complicated class that does need to work on multiple threads (mod caveats)? If it doesn't make sense to split the interface into multiple public objects, then it may make more sense to use an internal Core that lives on one thread, while your public interface lives on another. To call the Core, the Owner uses PostTaskAndReply[WithResult], binding the Response callbacks as WeakPtrs to the Owner. To delete the Core, the Owner uses DeleteSoon to delete the Core on the appropriate thread. If the Core wishes to call itself (via MessageLoop::PostTask), the Core uses a WeakPtrFactory<Core>, rather than this/base::Unretained(this). The ownership semantics are clear - Owner always owns the Core - which resolves the concerns with RefCounting (especially if you end up refcounting both Core and Owner)

Sorry, does that provide more context to the desired pattern?

[Kai Wang]

FYI, I just submitted a change modifying WeakPtr comments and unittests:

http://src.chromium.org/viewvc/chrome?view=rev&revision=146212

I think this makes the code easier to understand :)

[Wez]

Thanks for taking the time to tidy this up!

I think it would help to state up front the fundamental requirement that WeakPtrs only be dereferenced on the thread on which they will be invalidated, since the rest of the threading limitations are specific to the way the implementation tries to enforce that.

It might also make more sense for the comments to assume use of WeakPtrFactory, and then mention SupportsWeakPtr as a special-case, since we expect WeakPtr use to be more common as an internal detail of an implementation (using WeakPtrFactory) than as a public property of it (i.e. SupportsWeakPtr).

[Albert J. Wong (王重傑)]

patches welcome! ;)

[Kai Wang]

Thanks Wez. I do agree the comment should talk more about WeakPtrFactory, as it's more commonly used.

Please CC me the CL if you make any further modify :)

On Wed, Jul 11, 2012 at 5:26 PM, Wez <w...@chromium.org> wrote: