[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 #4 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 389079
  --> https://bugs.webkit.org/attachment.cgi?id=389079
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.

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

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

> 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

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

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:-82
> -    bool m_hasPendingWriteData { false };

I understand this patch tries to move the logic from this class to CurlSocketStreamXXX, but it seems a little bit too much. m_hasPendingWriteData is actually the mechanizm of WebKit side. It is okay to leave them here and make the change less in the platform layer side.

-- 
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/20200129/f570dd36/attachment.htm>