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

We tried to avoid blocking by using curl_multi_perform, but it causes some failures of layout tests (timeout).
Connections made by curl_multi_perform seem to fail detecting server close.
If you know about this, please let us know.

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

We haven't fixed.
Client can know if there is pending data by checking SocketStreamHandleImpl::m_hasPendingSendData.
Do you mean that SocketStreamHandleImpl::platformSendInternal should have vector of sending buffer and accept buffer regardless state of m_hasPendingSendData?


> 
> > Source/WebCore/platform/network/curl/CurlStream.cpp:153
> > +        destroyHandle();
> 
> Don't you need to start any closing sequence here?

We don't need to start closing here.
When WebSocketChannel::didReceiveSocketStreamData receives 0 byte buffer, it starts disconnecting.

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

Fixed.

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

Fixed.

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

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/20200202/96cd9722/attachment.htm>