[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 #34 from Takashi Komori <Takashi.Komori at sony.com> ---
(In reply to Basuke Suzuki from comment #28)
> Comment on attachment 389797 [details]
> 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.

Removed.

> 
> > 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.

Fixed.

> 
> > 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.

Added.

> 
> > 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.

Moved.

> 
> > 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.

Fixed.

-- 
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/20200212/7019931e/attachment.htm>