Thread race on incoming connections (findNextOpenClientSlot)

[hakusaro]

Reported by GameRoom on Slack:

I just found a bug with Terraria's netcode. It's somewhat specific to my server since it'd be rare to replicate outside of the stuff we're doing, but it's kind of general I guess. Essentially, if multiple players join the server at the exact same time, the findNextOpenClientSlot method can have 2 players go into the same slot since it's multithreaded
I found it on ours since we redirect all our players to different servers when they shut down. Other than that I doubt it could happen naturally but whatever
I put a lock statement around the contents of Netplay.OnConnectionAccepted and it works fine now

making a pull request

[8:48]  
oh apparently I don't have write access

[8:50]  
I just changed a few lines in Netplay.cs:
public static bool spamCheck = false;
public static bool anyClients = false;
**private static object slotLock = new object();**

...

        private static void OnConnectionAccepted(ISocket client)
        {
           **lock(slotLock)**
           {
               int num = Netplay.FindNextOpenClientSlot();
               if(num != -1)
               {
                   Netplay.Clients[num].Reset();
                   Netplay.Clients[num].Socket = client;
                   Console.WriteLine(client.GetRemoteAddress() + " is connecting...");
               }
               if(Netplay.FindNextOpenClientSlot() == -1)
               {
                   Netplay.StopListening();
               }
           }

[tylerjwatson]

When I get back I might take a look at this. Its not a good idea to place locks around I/O.

[hakusaro]

@tylerjwatson wolfje to the rescue <3

[hakusaro]

Pull request #108 has a potential solution for this. @tylerjwatson I'm not sure what you want to do for a potential fix that isn't locking, but there is certainly a PR now.

[ivanbiljan]

@hakusaro considering OTAPI and everything, is this still an issue?

[SignatureBeef]

Closing off as this is implemented.
See comments in PR: #112