[webkit-reviews] review granted: [Bug 180586] [WTF][Linux][GTK] Fix a minor bug in generic/WorkQueue when WorkQueue is immediately destroyed : [Attachment 328818] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180586: [WTF][Linux][GTK] Fix a minor bug in generic/WorkQueue when
WorkQueue is immediately destroyed
https://bugs.webkit.org/show_bug.cgi?id=180586

Attachment 328818: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 328818
  --> https://bugs.webkit.org/attachment.cgi?id=328818
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();
	});
    }