Add unit tests for TCPServer

[pafuent]

This PR aims to help "fix" #43440

[Faless]

It's always recommended to poll while waiting

[pafuent]

Could you please elaborate a little bit more?
A couple of lines below I'm calling client->poll(), so maybe I'm not fully getting how should I use poll.

[mhilbrunner]

@pafuent Great work and thanks for working on this! Not Fabio, and he may correct me, but:

Polling is what allows the connection (or underlying socket) to execute and do actual work. They don't run by themselves in parallel in threads or anything, they literally only do work when polled.

The server polls its socket when calling server->is_connection_available() (see here; I think we should actually point this one out more directly in TCPServer docs, looking at it), the client as you point out with client->poll().

This means that it is good practice to poll as regularly as possible, because the longer the time between polls:

• the higher the chance networking buffers may fill up/overrun and you lose packets/get errors
• the higher the chance you process messages too late (if you were to poll, say, every five seconds, you may end up processing information from 4 seconds ago that is by now outdated and no longer relevant, you don't really need the player's movement update from 4 seconds ago)
• the longer the connection appears to "hang" for the other side, as they won't get a packet response until you poll
• as per the above, this also introduces problems in transit, especially for TCP (which is not "fire and forget" like UDP), because both your node, the remote node and any node in between could decide your connection is dead because there has been no traffic, so why not kill the connection to free up resources? I.e., timeouts/connection terminations may happen, usually only with very high poll times, of course :)

Of course, all this is a bit more theoretical and "best practice" than 100% applicable to a test case like this. Instead of waiting arbitrarily for half a second, you could replace that with a loop that sleeps, say, 1000usecs, and then polls both client and server each time until you get your desired result (StreamPeerTCP::STATUS_CONNECTED), an error, or you run longer then expected and time out/fail the test.

Currently, this doesn't poll the connections for half a second, which means both ends don't process any network traffic for that time, which can be an eternity in networking. So, not really a good practice, although it works in this case because everything is local anyway and there is not much traffic.

Or, stated in another way: this test currently likely breaks if networking timeouts are set below 0.5 seconds (Which, granted, is pretty theoretical, but a local connection also should work faster than that) or we send more data and buffers are too small.

[pafuent]

Thanks for the amazing explanation!!!
In my case I needed to add that sleep because when I called poll or other method that does that internally I was getting a wrong return code or value. IIRC that was happening on Windows, but not on Mac or Linux.
Besides of that, if you think that it's worth it for the tests, I can try to change them to poll on a for loop with a smaller sleep time.

[Faless]

In my case I needed to add that sleep because when I called poll or other method that does that internally I was getting a wrong return code or value.

Indeed, the problem here is that you keep blocking for a long time and only poll once, while you should only block for brief periods of time and regularly poll (i.e. multiple times), or you risk of having the same error on other OS configurations.

[pafuent]

Friendly remainder

[pafuent]

I updated the test to only block for brief periods of time and regularly poll. Can I get the tests reviewed again?

[Geometror]

Only some minor code style issues left. Looks good overall, the implementation of the accept_connection function seems correct to me.

[pafuent]

@Geometror I fixed all the requested changes and also updated to the latest version of master
Can I bother you also with a review on #97311? (equivalent tests but for UDP)

[Calinou]

Looks good to me.

Awaiting approval by relevant team due to concerns over correctness to approve on style

[Faless]

As discussed with other members, I'm approving the PR, and will make the necessary changes in a separate PR.

[Repiteo]

Will need to be rebased before this can be merged

[akien-mga]

Thanks!