[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 #11 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 389232
  --> https://bugs.webkit.org/attachment.cgi?id=389232
Handling connections by one thread.

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

This is informal review.

> Source/WebCore/platform/network/curl/CurlStream.cpp:54
> +    auto errorCode = m_curlHandle->perform();

You cannot try connecting here. It blocks until it connect.

> Source/WebCore/platform/network/curl/CurlStream.cpp:87
> +    if (!m_curlHandle || m_writeBuffer)

This is wrong. If m_writeBuffer is not null, the contents of buffer is gone. And there's no way the client can know whether it can send or not. Add canSend() method or append data in the end of writeBuffer.

Or m_writeBuffer can be Vector<std::tuple<UniqueArray<uint_t>, size_t>>, then appending is just a simple task.

> Source/WebCore/platform/network/curl/CurlStream.cpp:153
> +        destroyHandle();

Don't you need to start any closing sequence here?

>> Source/WebCore/platform/network/curl/CurlStream.cpp:168
>> +    while (m_writeBufferOffset < m_writeBufferSize) {
> 
> Do you know the reason why 'while' loop is used here even though 'tryToRead' doesn't?

Yeah, this is wrong. The write() will block if send buffer is full. It was okay in a dedicate thread mode, but not okay in this patch. We can try just one timing after we get a permission to write after status check.

>> Source/WebCore/platform/network/curl/CurlStream.cpp:191
>> +void CurlStream::notifyFailWithError(CURLcode errorCode)
> 
> notifyFailWithError should be renamed because 'fail' is a verb, the phase 'notify fail' sounds weird.
> I think you refereed 'curlDidFailWithError'. the phase 'did fail' is not weird.

The verb `notify` is important for this method, simply using noun `Failure` and `notifyFailure` is enough, isn't it? `WithError` is apparent from the parameter.

> Source/WebCore/platform/network/curl/CurlStream.cpp:199
> +        client.didFailWithError(SocketStreamError(errorCode, url, CurlHandle::errorDescription(errorCode)));

Is this CurlScheduler's responsibility to instantiate SocketStreamError? It is good idea to be as ignorant as possible for WebKit implementation. Just return errorCode and url is enough, I think.

> Source/WebCore/platform/network/curl/CurlStream.cpp:203
> +void CurlStream::callClient(Function<void(Client&)>&& task)

Do we need this method? Each occasion to call client callback can call m_scheduler's method directly. That makes code a little bit busy, but more straight forward.

> Source/WebCore/platform/network/curl/CurlStream.h:46
> +        virtual void didOpen() = 0;

These interface method should have caller's instance as a first argument.

    virtual void didOpen(CurlStream&) = 0;

That is similar to Cocoa's delegation pattern. Adding that to each method gives more context to implementation side, i.e. who will require them. Also it may solve future method name conflict.

>> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:48
>> +    m_nextStreamID = (m_nextStreamID + 1 != invalidCurlStreamID) ? m_nextStreamID + 1 : 1;
> 
> You don't check m_nextStreamID is not used.
> Can you use the pointer of client instead of m_nextStreamID?

The timing of issuing ID and object allocation is different. We cannot simply use the address as an id. Also memory region can be reused for same object type and it is safe not to use address. But existence check is required.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:52
> +    auto task = [this, streamID, url = url.isolatedCopy()]() mutable {

This might be just a preference, but for me, assigning to temporal variable and use it in next line is not good rhythm to read. Calling `callOnWorkerThread()` with direct lambda is easy to read.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:70
> +        ASSERT(!isMainThread());

These assertion should be responsibility of callOnWorkerThead() or executeTask() method and no need to check in every places.

> Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:97
> +void CurlStreamScheduler::callOnMainThread(CurlStreamID streamID, WTF::Function<void(CurlStream::Client&)>&& task)

This is not general purpose invocation method. This function do more. How about callClientOnMainThread?

> Source/WebCore/platform/network/curl/CurlStreamScheduler.h:65
> +    HashMap<CurlStreamID, CurlStream::Client*> m_clientLists;

The value of item cannot be nullptr, so is should be CurlStream::Client&

-- 
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/20200131/23399e18/attachment-0001.htm>