The Curl platform code for WebSockets has not been implemented yet.
Created attachment 252386 [details] Patch
The patch passes all http websocket tests :)
Comment on attachment 252386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252386&action=review Code looks OK, but lots of room for improvement. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:57 > +class SocketStreamHandle : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase { Are we guaranteed that all the ref/deref are on the same thread? I am concerned that is not so. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:59 > + static PassRefPtr<SocketStreamHandle> create(const URL& url, SocketStreamHandleClient* client) { return adoptRef(new SocketStreamHandle(url, client)); } Should return Ref, not PassRefPtr. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:63 > +protected: A class that is never used as a base class should not have any protected members at all. These should all be private, not protected. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:69 > + bool readData(CURL* curlHandle); > + bool sendData(CURL* curlHandle); > + bool waitForAvailableData(CURL* curlHandle, int selectTimeoutMS); These CURL* don’t need argument names in the header, and also, should be CURL&. The selectTimeoutMS is an old fashioned choice and not what we should do in new code. A better type for the argument would be std::chrono::microseconds or perhaps std::chrono::milliseconds. Either way we would avoid all the comments about units and actually get compile time checking for it. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:72 > + static void worker(void*); This should be a lambda inside the member function that needs it, not a static member function. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:90 > + class SocketData { This seems like a peculiar class. Maybe it should just be a struct instead of a class. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:106 > + const char* m_data; This needs to be a std::unique_ptr<const char[]>; doing all the new and delete by hand is not a good idea. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57 > setClient(0); nullptr > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:100 > + char* data = new char[bufferSize]; Should use unique_ptr, not manual new/delete. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:108 > + callOnMainThread(didReceiveData, this); It would be better to use a lambda here rather than the mess with a static data member and a case of the data pointer inside it. If we had a thread safe reference count then we could use have the lambda capture a reference count to the handle, and then we would not need to use cancelCallOnMainThread in the destructor. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:222 > + SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context); This should be static_cast, not reinterpret_cast. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:233 > + auto receiveData = m_receiveData; > + m_receiveData.clear(); This should use WTF::move rather than copy and clear. Much more efficient and required if we change SocketData to read-only, which we should do. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:237 > + for (auto socketData : receiveData) { This needs to be auto& to avoid copying the socket data. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:252 > + SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context); This should be static_cast instead.
Thanks for reviewing :) Will upload another patch soon.
Created attachment 252612 [details] Patch
(In reply to comment #3) > > > Source/WebCore/platform/network/curl/SocketStreamHandle.h:69 > > + bool readData(CURL* curlHandle); > > + bool sendData(CURL* curlHandle); > > + bool waitForAvailableData(CURL* curlHandle, int selectTimeoutMS); > > These CURL* don’t need argument names in the header, and also, should be > CURL&. > I didn't change the parameter type, as CURL& didn't compile, since CURL is of type void.
Comment on attachment 252612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252612&action=review > Source/WebCore/platform/network/curl/SocketStreamHandle.h:49 > +#include <wtf/threading.h> This should have a capital "t". The headers here aren’t sorted alphabetically the way the WebKit style guide says they should be. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:87 > + class SocketData { This should just be a struct, not a class. I don’t see us getting any benefit from making it a class. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:89 > + SocketData(const char* data, int size) I think it’s too subtle that this takes ownership of the data. Making this a struct might help. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:100 > + SocketData(SocketData&& other) > + { > + m_data = WTF::move(other.m_data); > + m_size = other.m_size; > + other.m_size = 0; > + } Is it important that the other’s size gets set to 0 when moving? If not, then I suggest we just let the compiler generate this. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:104 > + ~SocketData() > + { > + } No need to define this. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:69 > + char* copy = new char[length]; This should go right into a std::unique_ptr when it’s allocated. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:101 > + m_receiveData.append(SocketData(data.release(), bytesRead)); This shows us we have a poor design for SocketData. It’s not good to call unique_ptr.release just to put something into another unique_ptr. If SocketData was a struct we could just write: m_receiveData.append(SocketData { WTF::move(data), bytesRead }); > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127 > + auto sendData = WTF::move(m_sendData.at(0)); > + m_sendData.remove(0); Should use takeFirst, not move/at/remove. And the fact that a Vector doesn’t have a takeFirst should make you realize that this should be a Duque, not a Vector. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:143 > + char* copy = new char[restLength]; Should go right into a std::unique_ptr. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:173 > + if (!m_workerThread) { Early return would be better than nesting the entire function inside an if. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:174 > + ref(); Need a comment explaining where the deref that matches this is. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:184 > + CString host = m_url.host().ascii(); The String::ascii() function should not be used unless there is a guarantee that all characters are ASCII. I don’t think there is a guarantee that all characters from URL::host will be ASCII. Maybe you want latin1() or utf8()? > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217 > + if (m_workerThread) { Early return would be better than nesting the entire function inside an if. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:218 > + m_stopThread = true; Probably needs to be an atomic bool, not just a normal bool. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220 > + m_workerThread = 0; nullptr
Created attachment 252729 [details] Patch
(In reply to comment #7) Thanks again for helpful review :) > > > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220 > > + m_workerThread = 0; > > nullptr I didn't change this, as m_workerThread does not seem to be of pointer type.
Comment on attachment 252729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252729&action=review lets not use this unnecessary std::unique_ptr > Source/WebCore/platform/network/curl/SocketStreamHandle.h:48 > +#if PLATFORM(WIN) > +#include <windows.h> > +#include <winsock2.h> > +#endif > + > +#include <curl/curl.h> > + > +#include <mutex> > + > +#include <wtf/Deque.h> > #include <wtf/RefCounted.h> > +#include <wtf/Threading.h> These aren’t sorted in the normal WebKit project way. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:88 > + const char* data() const { return m_data.get(); } > + int size() const { return m_size; } No need for these. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:90 > + std::unique_ptr<const char[]> m_data; Should just name this data. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:91 > + int m_size; Should just name this size. I also suggest initializing it to zero instead of leaving it uninitialized. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:99 > + Deque<std::unique_ptr<SocketData>> m_sendData; > + Deque<std::unique_ptr<SocketData>> m_receiveData; These should be Deque<SocketData> instead of Deque<std::unique_ptr<SocketData>). There’s no need for all the extra heap allocation. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51 > + startThread(); Should probably ASSERT(isMainThread()) here. But also, why do we need to start the thread? I don’t think we do. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57 > + stopThread(); Should just be ASSERT(!m_workerThread). The handle is ref'd as long as the thread is going. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:58 > + setClient(nullptr); I don’t understand the value of doing this. The setClient function has no side effects, and once the handle is deleted the null pointer can be overwritten by other random values. I don’t think this class needs a destructor at all, although perhaps the logging and assertions would be OK. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:70 > + std::unique_ptr<const char[]> copy(new const char[length]); > + memcpy(const_cast<char*>(copy.get()), data, length); Should be able to allocate a std::unique_ptr<char[]> and then pass it to a function expecting a std::unique_ptr<const char[]> and have it be converted. We’d avoid a const_cast that way. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:124 > + std::lock_guard<std::mutex> lock(m_mutexSend); Do we really need to lock during the actual send? This seems like a really long time to hold a lock. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:144 > + std::unique_ptr<const char[]> copy(new const char[restLength]); > + memcpy(const_cast<char*>(copy.get()), sendData->data() + totalBytesSent, restLength); > + m_sendData.prepend(std::unique_ptr<SocketData>(new SocketData { WTF::move(copy), restLength } )); Would be nice to share this code with platformSend. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:158 > + timeout.tv_sec = 0; > + timeout.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(selectTimeout).count(); This can do something crazy thing if selectTimeout is 1 second or more or if it’s negative. http://stackoverflow.com/questions/17402657/how-to-convert-stdchronosystem-clockduration-into-struct-timeval > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:188 > + CString host = m_url.host().utf8(); > + curl_easy_setopt(curlHandle, CURLOPT_URL, host.data()); No need for the local variable here. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200 > + if (refCount() > 1) > + didOpenSocket(); This check of refCount is peculiar. Is this an important optimization? > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217 > +void SocketStreamHandle::stopThread() This function needs an ASSERT(isMainThread())
Created attachment 252878 [details] Patch
(In reply to comment #10) > Comment on attachment 252729 [details] > Patch > Thank you for your patience :) > View in context: > https://bugs.webkit.org/attachment.cgi?id=252729&action=review > > lets not use this unnecessary std::unique_ptr > > > Source/WebCore/platform/network/curl/SocketStreamHandle.h:48 > > +#if PLATFORM(WIN) > > +#include <windows.h> > > +#include <winsock2.h> > > +#endif > > + > > +#include <curl/curl.h> > > + > > +#include <mutex> > > + > > +#include <wtf/Deque.h> > > #include <wtf/RefCounted.h> > > +#include <wtf/Threading.h> > > These aren’t sorted in the normal WebKit project way. > winsock2.h is needed before curl.h is included, so I kept this. > > > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51 > > + startThread(); > > Should probably ASSERT(isMainThread()) here. > > But also, why do we need to start the thread? I don’t think we do. > startThread() seems to be needed here, many tests timed out without it. > > > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:70 > > + std::unique_ptr<const char[]> copy(new const char[length]); > > + memcpy(const_cast<char*>(copy.get()), data, length); > > Should be able to allocate a std::unique_ptr<char[]> and then pass it to a > function expecting a std::unique_ptr<const char[]> and have it be converted. > We’d avoid a const_cast that way. > MSVC didn't want to convert this, I got the error: error C2664: 'std::unique_ptr<const char [],std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : cannot convert argument 1 from 'std::unique_ptr<char [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char [],std::default_delete<_Ty>> &&' > > > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200 > > + if (refCount() > 1) > > + didOpenSocket(); > > This check of refCount is peculiar. Is this an important optimization? > I'm also not too happy with this :) I added it to fix a crash where the call was invoked on the main thread after all other references were released, and the SocketStreamClient was deleted. Accessing the SocketStreamClient in didOpenSocket() then caused a crash. I had to add a SocketData constructor since m_sendData.append(SocketData { WTF::move(copy), length }) didn't compile with MSVC. The error I got was: error C2440 : '<function-style-cast>' : cannot convert from 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData' 1> No constructor could take the source type, or constructor overload resolution was ambiguous I also had to add a move constructor, otherwise I got an error stating that Deque.h (line 422) was trying to reference the deleted unique_ptr copy constructor.
> MSVC didn't want to convert this, I got the error: > > error C2664: 'std::unique_ptr<const char > [],std::default_delete<_Ty>>::unique_ptr(const > std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : > cannot convert argument 1 from 'std::unique_ptr<char > [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char > [],std::default_delete<_Ty>> &&' If this really is a bug in MSVC, could you use const_cast to add const instead of taking it away? That would be nicer. > I had to add a SocketData constructor since m_sendData.append(SocketData { > WTF::move(copy), length }) didn't compile with MSVC. The error I got was: > > error C2440 : '<function-style-cast>' : cannot convert from > 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData' > 1> No constructor could take the source type, or constructor > overload resolution was ambiguous Does it work to call the constructor without any initializer-list? (no {})
Comment on attachment 252878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252878&action=review > Source/WebCore/platform/network/curl/SocketStreamHandle.h:77 > + std::unique_ptr<const char[]> createCopy(const char* data, int length); Should be static. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:99 > + SocketData(std::unique_ptr<const char[]>&& source, int length) > + { > + data = WTF::move(source); > + size = length; > + } > + > + SocketData(SocketData&& other) > + { > + data = WTF::move(other.data); > + size = other.size; > + other.size = 0; > + } I don’t understand why you had to define these. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:71 > + m_sendData.append(SocketData(WTF::move(copy), length)); Should just be SocketData { WTF::move(copy), length } and then it should work without a constructor. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:215 > + if (refCount() > 1) > + didOpenSocket(); Needs a comment explaining why it’s here. Something like how you replied to my query about this.
Created attachment 253030 [details] Patch
(In reply to comment #14) > Comment on attachment 252878 [details] > Patch > Thanks again :) > > > Source/WebCore/platform/network/curl/SocketStreamHandle.h:99 > > + SocketData(std::unique_ptr<const char[]>&& source, int length) > > + { > > + data = WTF::move(source); > > + size = length; > > + } > > + > > + SocketData(SocketData&& other) > > + { > > + data = WTF::move(other.data); > > + size = other.size; > > + other.size = 0; > > + } > > I don’t understand why you had to define these. > > > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:71 > > + m_sendData.append(SocketData(WTF::move(copy), length)); > > Should just be SocketData { WTF::move(copy), length } and then it should > work without a constructor. > I'm sorry, but I don't seem to be able to get rid of these constructors. When I remove them, I get the following error: 1>C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xmemory0(611): error C2280: 'std::unique_ptr<char [],std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function 1> with 1> [ 1> _Ty=char [] 1> ] 1> C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1618) : see declaration of 'std::unique_ptr<char [],std::default_delete<_Ty>>::unique_ptr' 1> with 1> [ 1> _Ty=char [] 1> ] 1> This diagnostic occurred in the compiler generated function 'WebCore::SocketStreamHandle::SocketData::SocketData(const WebCore::SocketStreamHandle::SocketData &)' ========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ========== I believe this happens because Deque::append and Deque::prepend makes MSVC generate a SocketData copy constructor, which is not possible since the copy constructor in unique_ptr is deleted. There seems to be reports of similar problems: https://social.msdn.microsoft.com/Forums/vstudio/en-US/ace575e6-3969-4672-9db1-85c9d6588e4a/cant-put-class-with-uniqueptr-member-in-stl-container?forum=vclanguage http://stackoverflow.com/questions/10133591/using-smart-pointers-in-a-struct-or-class Is this a MSVC problem? I assume gcc doesn't have these problems.
(In reply to comment #13) > > MSVC didn't want to convert this, I got the error: > > > > error C2664: 'std::unique_ptr<const char > > [],std::default_delete<_Ty>>::unique_ptr(const > > std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : > > cannot convert argument 1 from 'std::unique_ptr<char > > [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char > > [],std::default_delete<_Ty>> &&' > If this really is a bug in MSVC, could you use const_cast to add const > instead of taking it away? That would be nicer. > > I had to add a SocketData constructor since m_sendData.append(SocketData { > > WTF::move(copy), length }) didn't compile with MSVC. The error I got was: > > > > error C2440 : '<function-style-cast>' : cannot convert from > > 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData' > > 1> No constructor could take the source type, or constructor > > overload resolution was ambiguous > Does it work to call the constructor without any initializer-list? (no {}) Thanks for looking into this :) I removed the const modifier from the unique_ptr member in the last patch to avoid all the casting. I hope this is acceptable :)
(In reply to comment #16) > I believe this happens because Deque::append and Deque::prepend makes MSVC > generate a SocketData copy constructor, which is not possible since the copy > constructor in unique_ptr is deleted. As you say, I think this is indeed an MSVC bug.
Comment on attachment 253030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253030&action=review > Source/WebCore/platform/network/curl/SocketStreamHandle.h:92 > + SocketData(std::unique_ptr<char[]>&& source, int length) > + { > + data = WTF::move(source); > + size = length; > + } Really irritating that we have to do this to work around a bug in MSVC, but I’m pretty sure that is indeed what we are doing, working around a bug. Two separate related bugs, I guess. > Source/WebCore/platform/network/curl/SocketStreamHandle.h:99 > + SocketData(SocketData&& other) > + { > + data = WTF::move(other.data); > + size = other.size; > + other.size = 0; > + } Really irritating that we have to do this to work around a bug in MSVC, but I’m pretty sure that is indeed what we are doing, working around a bug. Two separate related bugs, I guess.
(In reply to comment #19) > Comment on attachment 253030 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253030&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandle.h:92 > > + SocketData(std::unique_ptr<char[]>&& source, int length) > > + { > > + data = WTF::move(source); > > + size = length; > > + } > > Really irritating that we have to do this to work around a bug in MSVC, but > I’m pretty sure that is indeed what we are doing, working around a bug. Two > separate related bugs, I guess. > Thanks for reviewing :) I can try to remember to revisit this code when we move to the next MSVC version.
Committed r184389: <http://trac.webkit.org/changeset/184389>