4 ms·
Hey there. Just wrote down a few things that came to my mind after seeing this. Don't see it as criticism, but as a few hints/remarks what has to be covered by
by Matthias247 10y ago
Hey there. Just wrote down a few things that came to my mind after seeing this. Don't see it as criticism, but as a few hints/remarks what has to be covered by a websocket (or any other protocol) implementation. Imho the high performance part isn't too hard to achieve and shouldn't be the highest rated. The important thing is that such a server should be rock solid in implementation, otherwise it's worthless.
Some things that you should answer if you are offering this as a C++ websocket library, and which are currently not covered in the header file:
- Whats the threading model of the library?
- Will server.run() start a singlethreaded eventloop (I guess so from taking a supershort peek into the code and seeing libuv) and everything is running inside there or will it start multiple worker threads?
- Based on the last question, from which thread[s] are the other callbacks called.
- If multiple threads are used, is the library threadsafe for sending messages from other threads
- Is it integratable in other eventloops? Most applications already have a mainloop or something like this, libraries which only work with their own mainloop are not very useful. Normally applications also have to deal with other application logic besides responding to websocket messages.
Besides that a few general questions that you should be able to answer for a websocket implementation:
- What's the sending behavior of socket.send?
- Will it block until all data was sent? This can cause problems with slow receivers in singlethreaded environments.
- Will it copy all data and buffer it internally until it can be sent? This provides no means for backpressure and slow receivers (or non-receivers) can exhaust the servers memory.
- Does it handle connection close properly? This is unfortunatly not too easy in websockets.
- And are there timers in place for force closing the connection if the shutdown sequence is not completed properly? Or if the initial handshake is not completed in a given timeframe?
- Does ist handle control frames? And will it merge control frames (PONGs) if multiple are queued before they are sent? And will it stop sending them after close connection is initiated?
- toomim 10y agoFor your first set of questions: it's single threaded. If you want use multiple processors/threads, you start multiple servers, and either distribute loads with a load balancer or give each one a different TCP port.
- alexhultman 10y agoThe server is async, so there is no blocking functions exposed. It passes all Autobahn tests, meaning it properly handles close frames & pings etc. Timers are used to force close connections. The C++ HTTP server does not currently time out, but the Node.js HTTP server does, so this is one issue that needs to be fixed, yes.
- Matthias247 10y agoJust looked at the code. You seem to queue everything and therefore never block. That can be OK for some use-cases, but you don't provide any kind of backpressure due to that in the send command. It will get problematic in case of slow receivers. And in node it will break the stream semantics. E.g. if someone pipes 1Gb of messages into your socket (or sends them if WriteableStream is not supported) he will think that they have been sent immediatly and won't know that the data is buffered on lower layer. If you pipe a fast source into a slow receiver it will break over time - which is exactly the thing that node streams actually try to avoid. And another thing I saw there: Your SHORT_SEND optimization looks broken, as there does not seem to be tracked if the buffer is already used by another message that is still queued for sending. So short messages can corrupt each other.
- alexhultman 10y agoI can assure you, the short send optimization is not broken.
- Matthias247 10y agoOk, the buffer seems tob be copied it in the queuing code if it needs to be queued. That's fine. But it would have been helpful to point to that code lines instead of only "assuring".