[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 #6 from Takashi Komori <Takashi.Komori at sony.com> ---


(In reply to Basuke Suzuki from comment #4)
> Comment on attachment 389079 [details]
> Handling connections by one thread.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389079&action=review
> 
> This is an informal review. So to describe the overview of the changes,
> - CurlSocketHandle code was moved to CurlSocketStreamWorker,
> - SocketStreamHandleImpl code was moved to CurlSocketWorkerManager and
> CurlSocketStreamHandle
> 
> The instance of CurlSocketStreamHandle exists only in main thread and those
> of CurlSocketStreamWorker are in worker thread. This is a good separation
> and very effective way to prevent thread related memory issue.
> 
> I think CurlSocketStreamHandle do a little bit too much. It should be
> thinner layer as name implies and tasks should be asked to WorkerManager
> with an argument of Handle. Stop registering itself to manager, but let
> manager create Handle and register it by itself.
> 
> Last thing, class names are too long and hard to get the meaning. I suggest
> renaming like this:
> - CurlSocketStreamWorkerManager -> CurlStreamScheduler (corresponding to
> CurlRequestSchedular)
> - CurlSocketStreamWorker -> CurlStream
> - CurlSocketStreamHandle -> CurlStream::Handle
> - CurlSocketStreamHandleClient -> CurlStream::Client
> 
> Because this is inside curl network layer implementation, there's nothing to
> point to Stream since today. We can safely use simple term "Stream" for the
> meaning of SocketStream.
> 
> Also as WinCairo also has an implementation of RemoteInspectorSocket and the
> term "Socket" is used everywhere around that, I feel better not seeing that
> term  in Curl layer.
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:120
> > +    m_socketStreamWorkerManager = makeUnique<CurlSocketStreamWorkerManager>();
> 
> It should be lazy initialization.

FIXED.

> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:259
> > +CURLMcode CurlMultiHandle::poll(curl_waitfd* extraFds, unsigned numOfExtraFds, int timeoutMS, int& numFds)
> 
> This isn't used in this patch. Please remove it.

REMOVED.

> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:-874
> > -#ifndef NDEBUG
> 
> These changes happens because of the layout change of functions. Please try
> minimizing the diff size.

FIXED.

> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:886
> > +        errorHandler(errorCode);
> 
> errorHandler is used just for returning errorCode. There must be better way
> to do that. i.e. WTF::Expected

FIXED.

> 
> > Source/WebCore/platform/network/curl/CurlSocketStreamHandleClient.h:32
> > +class CurlSocketStreamHandleClient {
> 
> This class is very close to CurlSocketStreamHandle and recent implementation
> tend to put that as a embedded class. Let's move it inside
> CurlSocketStreamHandle . like:
> class CurlSocketStreamHandle {
> ...
> public:
> ...
>     class Client {
>         ...
>     }
> 
> Then this file can be deleted and the reader can understand the relationship
> easily afterword.
> 
> > Source/WebCore/platform/network/curl/CurlSocketStreamWorkerClient.h:35
> > +class CurlSocketStreamWorkerClient {
> 
> I don't think this abstraction is required. It's too much. WorkerManager
> should handle Workers directly.

Reduced files by introducing CurlStream::Client class.

-- 
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/20200130/df5b3fb8/attachment.htm>