Threading revision in cClientHandle

[xoft]

Hi all,

I'm thinking of rewriting the cClientHandle thread usage. Currently the server needs 2 threads per player connected. This is quite inefficient and may pose a limit on the number of clients connected (since memory is used for each thread's stack, limiting the number of threads on 32-bit systems to about 2000).

I'm proposing the following change:
One object, cSocketThreads, will maintain a group of threads responsible for handling socket input and output. One such pair of threads would handle( FD_SETSIZE - 1) sockets (63 on windows, usually 1023 on linux). The input thread will only read from sockets, parse the byte stream into packets and call each cClientHandle's HandlePacket() for each of the packets. The output thread will wait for data in the outgoing-packet-queue, then query sockets' readiness for writing and write packets to available sockets.

From the outside, the interface would be this:
cSocketThreads::AddClient(cSocket * iSocket, cClientHandle * iClient) - add a socket for processing, data from the sockets are to be handled by iClient.
cSocketThreads::RemoveClient(cSocket * iSocket) - remove the socket (and associated client) from processing
cSocketThreads::RemoveClient(cClientHandle * iClient) - remove the associated socket and the client from processing
cSocketThreads::QueuePacket(cClientHandle * iClient, cPacket * iPacket) - puts a packet into the outgoing-packet-queue for sending to the specified client

Internally, the cSocketThreads will create objects of type cSocketThread, each of which will handle one input and one output thread, and maintain the list of sockets. One internal socket will be used for maintenance - when adding or removing a client or queueing a packet, this socket will be written so that the accept() call gets unblocked and the internal socket list can be updated, or the packet queue examined.

Initially I wanted to use only input threads and send output directly to the client (calling send() in cClientHandle::Send() ). The reason not to do this is that if there is a slow or malicious client, it would cause the server to gradually block on each thread that does some sending. FakeTruth says this has been a real issue before, that's why the SendThread in cClientHandle was implemented.

Also this change will require rewriting part of packet parsing. The parsing functions now need to read from a string, rather than a socket, and report three states - success, error in data, and incomplete packet. In the case of success, they'll also need a way to indicate number of bytes consumed from the input stream. This can be easily achieved by using an int return value meaning number of bytes consumed if positive, zero if incomplete packet and negative in case of a protocol error.

One thing that has to be available in the new design is the ability to filter packets (currently done in cClientHandle::Send()) - when there are several EntityMoveLook packets for the same entity, they get eliminated but for the last one. This is important, since such a packet may be sent for each entity every tick (10 bytes of packet * 200 entities * 20 times a second = 40 KB/s) *per each player*.
In the proposed design, the cSocketThreads' QueuePacket will still be capable of filtering packets this way.

I'd like to hear your opinions on this change.

EDITed: clarified a bit and re-calculated the EntityMoveLook bitrate

[FakeTruth]

Do it.

It seems like you're promising wonderful things without any disadvantages, so I say go for it

[xoft]

Actually, it gets even better, we don't need a pair of threads, we can do with just one thread for both reading and writing. I hope my thinking is correct on this one, please correct me if you find anything bad in this:

1. Put clients' sockets into the Read and Error sets (signalled when there's incoming data or the socket is lost)
2, Wait for the select() to return, with arbitrary timeout.
3, When it returns, check the sockets for incoming data, and read it.
4, If there are any packets queued for sending, perform a write-check:
5, Put all sockets into a Write set (signalled when writing is available) and call select() with zero timeout
6, Sockets that can be written to get the packets from the queue written to them
7, Go to 1 again

This should handle reading efficiently, but has a problem with writing - when a packet is queued, this loop will still wait for incoming data in step 1. So we need a way to unblock the select() call in #1. We do this by creating a loopback socket (localhost to localhost) and sending data over it , when a packet is queued. This Control socket is the reason for the "minus one" in FD_SETSIZE - 1.

Also, we need to handle socket adding and removing, this will use the same mechanism - using the Control socket to "unblock" the select() call.

[xoft]

I did the first half of the task. I rewrote all the packets (ugh) for streaming and then made cServer use cSocketThreads for the client connections, but for now they're used only for reading. I'd be grateful if more people would test this (rev 244), as I could have overlooked something, potentially creating a crash or deadlock ability in some unusual circumstances. If I get no problem reports, I'll finish the sending part of this task tomorrow.

[FakeTruth]

I won't be able to test much since I'm going on vacation tomorrow, I'll be back on Monday I think.

[xoft]

I can see one side-effect that I haven't observed before, but it might just be that I didn't check too much.
A player can place a block that collides with their bounding box (i. e. under their feet or into their face), the server doesn't make any checks against this.
Can anyone confirm whether this is new behaviour since rev 244 or has this been observed before?

[FakeTruth]

Using the Core plugin fixes this

[xoft]

Not really, since Core only checks players, not mobs. And why do we implement such a basic game logic in a plugin?

[FakeTruth]

I don't know? Maybe some people want to be able to place blocks into other people