[Webkit-unassigned] [Bug 187984] [Curl] Use shared single thread for WebSocket connections

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=187984

--- Comment #28 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 389797
  --> https://bugs.webkit.org/attachment.cgi?id=389797
Handling connections by one thread.

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

This is another informal review from the previous implementer of this logic. This time, checking the thread lock area.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:118
> +        auto locker = holdLock(m_mutex);

Just after waitForCompletion(), it is ensured the thread is not running here. No need to lock.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:126
> +        m_runThread = false;

workerThread() never return from looping until m_runThread == false. I think there's no need to set the variable here. This makes entire life cycle unclear and very confusing.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:131
> +{

Add assert to ensure called in worker thread. And let code reader (me) know about that explicitly.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:133
> +    if (m_taskQueue.size() || m_streamLists.size())

Move m_streamLists.size() check before locking to avoid unneeded lock.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:162
> +            auto locker = holdLock(m_mutex);

m_runThread is set to false only within worker thread. There's no need to lock.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:163
> +            if (!m_runThread)

Then this check can be move into `while`'s condition.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200207/8cc74fe2/attachment.htm>