>> In bug 1218297 we saw a case where dispatch to a thread (the socket
>> transport service thread in this case) fails because the thread has
>> already shut down. In our brave new world, nsThread simply leaks the
>> runnable. It can't release the reference it holds, because that would
>> reintroduce the race condition we wanted to avoid, and it can't
>> release the reference on the correct thread so it's already gone away.
>>
>
>I am a bit confused with this, if the executing thread has already shut
>down, why would releasing the reference dispatching thread holds
>reintroduce the race condition? Who is racing with dispatching thread?
It doesn't re-introduce a race. What it may do is cause the same effect
as a lost race, whereby an object meant to be released on the target
thread gets released on the sending thread. If the object is a
non-threadsafe object (say, for example, a JS success or failure
callback object), then releasing it on the sending thread is Bad. This
is the same thing that can happen in a race where you have:
What used to happen:
{
RefPtr<Foo> bar = new FooRunnable(nonthreadsafe_object);
target_thread->Dispatch(bar, ...);
// Note: Dispatch in this (old-style) case takes a new reference to
// bar. And the runnable may run and release that reference before
// Dispatch() returns! And if Dispatch() fails, you always 'lose'
// the race.
}
// bar drops the reference. If the runnable ran already, this is
// the last reference and it gets destroyed here. If it hasn't run,
// it gets destroyed on the target thread.
New way (already_AddRefed<>):
{
RefPtr<Foo> bar = new FooRunnable(nonthreadsafe_object);
target_thread->Dispatch(bar.forget(), ...);
// bar is always destroyed on the target thread - unless
// Dispatch() fails, in which case it leaks (for safety).
}
or:
other_thread->Dispatch(do_AddRef(new FooRunnable(nonthreadsafe_object)),
...);
If you use the old-style (raw ptr Dispatch) as a lot of the tree still
does, it will now leak on Dispatch() failure instead of possibly
wrong-thread releasing:
nsThread.h:
nsresult Dispatch(nsIRunnable* aEvent, uint32_t aFlags) {
return Dispatch(nsCOMPtr<nsIRunnable>(aEvent).forget(), aFlags);
}
Since Dispatch(already_AddRefed<>,...) leaks on failure, this means that
Dispatch(raw_ptr,...) now leaks on failure (which is what engendered
this discussion). You can override this behavior with a check of the
result of Dispatch() and a manual Release if it fails (and you know
the object is entirely threadsafe).
We could add DispatchFallible() (DispatchNeverLeaks()?) if that's a
wide-enough pattern; part of my question in that case is "did the caller
really expect Dispatch to possibly fail, and what implications does such
a failure have to the rest of the system?"
--
Randell Jesup, Mozilla Corp
remove "news" for personal email
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
