Bug 180586 - [WTF][Linux][GTK] Fix a minor bug in generic/WorkQueue when WorkQueue is immediately destroyed
Summary: [WTF][Linux][GTK] Fix a minor bug in generic/WorkQueue when WorkQueue is imme...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-08 08:49 PST by Yusuke Suzuki
Modified: 2017-12-08 10:38 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2017-12-08 08:51 PST, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-12-08 08:49:57 PST
[WTF] Fix a minor bug in generic/WorkQueue when WorkQueue is immediately destroyed
Comment 1 Yusuke Suzuki 2017-12-08 08:51:54 PST
Created attachment 328818 [details]
Patch
Comment 2 Yusuke Suzuki 2017-12-08 09:33:14 PST
I also wonder if we can use generic/WorkQueue for Windows, I need to check the implementation.
Comment 3 Darin Adler 2017-12-08 10:21:56 PST
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();
        });
    }
Comment 4 Yusuke Suzuki 2017-12-08 10:23:43 PST
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.
Comment 5 Yusuke Suzuki 2017-12-08 10:37:13 PST
Committed r225684: <https://trac.webkit.org/changeset/225684>
Comment 6 Radar WebKit Bug Importer 2017-12-08 10:38:19 PST
<rdar://problem/35937775>