Bug 180333 – [WTF] Thread::create should have Thread::tryCreate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Thread::create should have Thread::tryCreate

[Yusuke Suzuki]

Created attachment 328294 [details]
Patch

[Darin Adler]

Comment on attachment 328294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328294&action=review

The move from RefPtr to Ref is a nice cleanup.

But as for tryCreate, I don’t fully understand this. Do we really think it’s OK for WebKit to run without some threads that it needs? Maybe we have a discipline where all threads allocated first are required and then later we can run in a thread-starved state with some operations failing, but others continuing to work correctly?

Normally with memory allocation, for example, very few clients are actually prepared to have a second path if allocation fails. It’s the ones that are allocating large amounts of data, typically proportional to end user data sets, and thus those are written to understand the possibility of failure. But small allocations are assumed to never fail.

I don’t see two separate categories like this for threads. There aren’t cheaper threads and more costly threads, so we can’t assume that cheaper thread allocations will succeed after the more costly ones.

It’s OK to go on this path if we have a clear goal in mind, but I am not sure the goal is clear, at least I don’t understand it.

> Source/WebCore/platform/audio/ReverbConvolver.cpp:66
> -    , m_backgroundThread(0)
> +    , m_backgroundThread(nullptr)
>      , m_wantsToExit(false)
>      , m_moreInputBuffered(false)

Can we initialize these in the class definition?

> Source/WebCore/workers/WorkerThread.cpp:143
> +    m_thread = Thread::tryCreate("WebCore: Worker", [this] {

Why is it OK to change the semantics of WorkerThread::start? Doesn’t it need to be renamed to tryStart if it can fail? Do callers really handle the case where it fails?

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:222
> +    m_syncThread = Thread::tryCreate("WebCore: IconDatabase", [this] {

Seems that this function is written half-claiming it can handle a failure to create a thread, but not really. For example, if this open function returns false, it’s not OK that you then have to call the close function. But we have already done m_mainThreadNotifier.setActive(true) here, which seems to indicate I need to call close.

I think that maybe this isn’t quite right when m_syncThread ends up null.

> Source/WebKitLegacy/Storage/StorageThread.cpp:57
> +        m_thread = Thread::tryCreate("WebCore: LocalStorage", [this] {

Same question: Why is it OK to change the semantics of StorageThread::start? Doesn’t it need to be renamed to tryStart if it can fail? Do callers really handle the case where it fails?

Maybe we should not add this thread to activeStorageThreads() if we weren’t able to create a thread? Again, not sure what the concept is here.

> Source/WebKitLegacy/win/WebKitQuartzCoreAdditions/CVDisplayLink.cpp:-67
> -    ASSERT(m_ioThread);

Why remove this assertion? Doesn’t seem necessary to remove it.

[Yusuke Suzuki]

Comment on attachment 328294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328294&action=review

The problem I attempt to solve with this patch is that there are clients that do not null check of the `RefPtr<Thread>`.
If the clients think that should never fail, failing early with RELEASE_ASSERT is safer.

This patch is introducing `Thread::create` and `Thread::tryCreate`. `Thread::tryCreate` is the same one to the current `Thread::create`. It returns `RefPtr<Thread>`. But new `Thread::create` performs RELEASE_ASSERT and returns `Ref<Thread>`.
Using `Thread::create` with RELEASE_ASSERT is safer way for clients that think thread is mandatory. It enforce the nullptr check.

So basically, this patch attempts to insert RELEASE_ASSERT to the result of the current `Thread::create`. This patch changes only several places intentionally to using `Thread::tryCreate` because I see the explicit handling of nullptr is described in these code. But I'm not sure this handling is reasonable. Anyway, in this patch, I keep these code as is.

I think we should revisit these code. I'm not sure whether these nullptr handlings are reasonable ones. At that time, `Thread::tryCreate` is a good marker for these code. And it explicitly describes that the code handles the failure case of the thread creation.

>> Source/WebCore/platform/audio/ReverbConvolver.cpp:66
>>      , m_moreInputBuffered(false)
> 
> Can we initialize these in the class definition?

Fixed.

>> Source/WebCore/workers/WorkerThread.cpp:143
>> +    m_thread = Thread::tryCreate("WebCore: Worker", [this] {
> 
> Why is it OK to change the semantics of WorkerThread::start? Doesn’t it need to be renamed to tryStart if it can fail? Do callers really handle the case where it fails?

This code does not change the semantics because the old `Thread::create` is the same to the `Thread::tryCreate`.
This patch is not attempting to change the semantics of the current code. Since this `start()` has the handling of nullptr thread case, this patch uses `Thread::tryCreate` here.
I'm not sure whether the failure case of `WorkerThread::start` is well handled. We should revisit, but this is not the intension of this patch. Rather, this patch makes it explicit by renaming the old `Thread::create` with `Thread::tryCreate`.

>> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:222
>> +    m_syncThread = Thread::tryCreate("WebCore: IconDatabase", [this] {
> 
> Seems that this function is written half-claiming it can handle a failure to create a thread, but not really. For example, if this open function returns false, it’s not OK that you then have to call the close function. But we have already done m_mainThreadNotifier.setActive(true) here, which seems to indicate I need to call close.
> 
> I think that maybe this isn’t quite right when m_syncThread ends up null.

Interesting. And I think exploiting this is the intension of this patch. This patch uses `Thread::tryCreate` only when the code has the failure case of `Thread::create`.
But these places should be revisited.

>> Source/WebKitLegacy/Storage/StorageThread.cpp:57
>> +        m_thread = Thread::tryCreate("WebCore: LocalStorage", [this] {
> 
> Same question: Why is it OK to change the semantics of StorageThread::start? Doesn’t it need to be renamed to tryStart if it can fail? Do callers really handle the case where it fails?
> 
> Maybe we should not add this thread to activeStorageThreads() if we weren’t able to create a thread? Again, not sure what the concept is here.

Ditto. This change does not change the semantics. The old `Thread::create` is the same to the new `Thread::tryCreate`.

>> Source/WebKitLegacy/win/WebKitQuartzCoreAdditions/CVDisplayLink.cpp:-67
>> -    ASSERT(m_ioThread);
> 
> Why remove this assertion? Doesn’t seem necessary to remove it.

This is what this patch is doing. The bad thing is that the old `Thread::create` returns `RefPtr<Thread>`, not `Ref<Thread>`. The new `Thread::create` returns `Ref<Thread>` by ensuring the success of the thread creation by using `RELEASE_ASSERT`. So, now, m_ioThread becomes `Ref<Thread>`, and this assertion is not necessary since it is exactly done in the new `Thread::create`.

[Yusuke Suzuki]

Created attachment 328449 [details]
Patch

[Yusuke Suzuki]

Created attachment 328451 [details]
Patch

[Darin Adler]

Comment on attachment 328451 [details]
Patch

Seems fine. We should also come back and change every last one of these from tryCreate to create. I am not convinced that any of these really want to stay with tryCreate.

[Yusuke Suzuki]

Committed r225778: <https://trac.webkit.org/changeset/225778>

[Radar WebKit Bug Importer]

<rdar://problem/35990798>