Bug 180586 – [WTF][Linux][GTK] Fix a minor bug in generic/WorkQueue when WorkQueue is immediately destroyed

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[WTF] Fix a minor bug in generic/WorkQueue when WorkQueue is immediately destroyed

[Yusuke Suzuki]

Created attachment 328818 [details]
Patch

[Yusuke Suzuki]

I also wonder if we can use generic/WorkQueue for Windows, I need to check the implementation.

[Darin Adler]

Comment on attachment 328818 [details]
Patch

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

> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:50
> +    // Ensure RunLoop is already starting. Otherwise, platformInvalidate's RunLoop::stop() could be missed.
> +    m_runLoop->dispatch([&] {
> +        semaphore.signal();
>      });

Could this work be done inside Thread::create right after calling RunLoop::run? Then we would only need one Semaphore::signal/wait pair.

> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:51
> +    semaphore.wait(WallTime::infinity());

Is this synchronous delay OK? Either it’s so tiny that it doesn’t matter, or it could potentially be an unwanted performance cost for code that initializes work queues. An alternative would be to have the semaphore be a data member and do the wait only in platformInvalidate. Not as elegant in some ways, but more direct in other ways.

Or could we solve the same problem with something like this in platfomInvalidate?

    if (m_runLoop) {
        auto runLoop = m_runLoop;
        runLoop->stop();
        runLoop->dispatch([runLoop] {
            runLoop->stop();
        });
    }

[Yusuke Suzuki]

Comment on attachment 328818 [details]
Patch

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

>> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:50
>>      });
> 
> Could this work be done inside Thread::create right after calling RunLoop::run? Then we would only need one Semaphore::signal/wait pair.

RunLoop::run() does not return until RunLoop::stop() is called...

>> Source/WTF/wtf/generic/WorkQueueGeneric.cpp:51
>> +    semaphore.wait(WallTime::infinity());
> 
> Is this synchronous delay OK? Either it’s so tiny that it doesn’t matter, or it could potentially be an unwanted performance cost for code that initializes work queues. An alternative would be to have the semaphore be a data member and do the wait only in platformInvalidate. Not as elegant in some ways, but more direct in other ways.
> 
> Or could we solve the same problem with something like this in platfomInvalidate?
> 
>     if (m_runLoop) {
>         auto runLoop = m_runLoop;
>         runLoop->stop();
>         runLoop->dispatch([runLoop] {
>             runLoop->stop();
>         });
>     }

That sounds better! I'll fix it.

[Yusuke Suzuki]

Committed r225684: <https://trac.webkit.org/changeset/225684>

[Radar WebKit Bug Importer]

<rdar://problem/35937775>