Your example is suffering from the fact that PR_CreateThread is taking
as parameter an object that is half-initialized, and then continues
constructing it. I believe this to be a poor design and that
unfortunately leaks into the creating of nsThread.
In such a situation I would personally still use a static Create method
which would go something like:
static
already_addRefed<nsThread> Create(...)
{
nsRefPtr<nsThread> thread = new nsThread(...);
if (!thread->Init()) {
// the destructor still has to account for failed init but oh
well...
return nullptr;
}
// at least no external code will get access to a nsThread that is not
fully initialized
return thread.forget();
}
with the constructor and Init() method protected.
It's not as good as not having to run the destructor on a partially
invalid object, but it is way less foot-gun prone for external code,
since they won't see a half-initialized nsThread and maybe forget to
call IsValid(), Init() or whatnot.
We had a lot of code in gfx that was using the constructor then Init()
and constructor then check patterns, and we ran into bugs related to
using invalid objects. Eventually we converted a lot of that code to the
static Create() style and I am very happy about how simpler and safer
things got as a result.
In my opinion, making it impossible by construction to get a pointer to
a half-initialized or partially invalid object is the important part. If
you can avoid instantiating the object all together in case of failure
it's perfect, if you have to do the instantiation because poor code
forces that on you, it's not perfect, but I think it is still better
than the risk of passing invalid objects around.
Cheers,
Nical
On Thu, Apr 21, 2016, at 05:05 PM, Nicholas Nethercote wrote:
> On Thu, Apr 21, 2016 at 8:41 PM, Martin Thomson <m...@mozilla.com> wrote:
> >
> >> - I suspect that in some heap-allocated objects it will be hard to do
> >> the fallible parts without having an object of the relevant type. I
> >> don't have specific examples, just a hunch.
> >
> > I'm not aware of any way that necessary state can't be fully
> > established before constructing an object. If you find a way in which
> > this is possible, please let me know.
>
> I just tried this pattern in three classes I've been experimenting
> with and hit problems in all three.
>
> In bug 1265965 I wrote patches to convert nsThread+Init style to the
> nsThread+outparam style, which worked well. So I tried the style
> Nicolas and you are advocating, which I'll calls
> "falllible-then-infallible". Here is the Init() function, from the
> current trunk code:
>
> nsresult
> nsThread::Init()
> {
> // spawn thread and wait until it is fully setup
> RefPtr<nsThreadStartupEvent> startup = new nsThreadStartupEvent();
>
> NS_ADDREF_THIS();
>
> mShutdownRequired = true;
>
> // ThreadFunc is responsible for setting mThread
> PRThread* thr = PR_CreateThread(PR_USER_THREAD, ThreadFunc, this,
> PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD,
> PR_JOINABLE_THREAD, mStackSize);
> if (!thr) {
> NS_RELEASE_THIS();
> return NS_ERROR_OUT_OF_MEMORY;
> }
>
> // ThreadFunc will wait for this event to be run before it tries to
> access
> // mThread. By delaying insertion of this event into the queue, we
> ensure
> // that mThread is set properly.
> {
> MutexAutoLock lock(mLock);
> mEventsRoot.PutEvent(startup, lock); // retain a reference
> }
>
> // Wait for thread to call ThreadManager::SetupCurrentThread, which
> completes
> // initialization of ThreadFunc.
> startup->Wait();
> return NS_OK;
> }
>
> So this is the code that must run before we construct an actual
> nsThread object, right? The PR_CreateThread() call is the main
> problem. It takes |this| as a parameter and proceeds to do all sorts
> of stuff with it. It's probably not *impossible* to rewrite this as
> fallible-then-infallible, but I'm struggling to see what it would look
> like.
>
> The second example I looked at was nsScriptSecurityManager. The
> constructor+outparam style worked well there, but
> fallible-then-infallible again hits a problem -- InitPrefs() is called
> in the fallible section of initialization, and it calls
> ScriptSecurityPrefChanged(), which touches lots of class members.
> Maybe InitPrefs() could be moved out of the fallible section, but I
> don't know this code well enough to say one way or the other.
>
> The third example I looked at was CycleCollectedJSRuntime. Again
> problems, specifically this line in Init():
>
> mOwningThread->SetScriptObserver(this);
>
> So, for all three examples I looked at fallible-then-infallible had
> problems ranging from moderate to severe. My conclusion is that
> partially initializing an object when the object doesn't exist is
> difficult, and, as a result, fallible-then-infallible is not a
> reliable general-purpose pattern.
>
> Nick
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
