0

I have a server set up to listen on a port for clients, then if they find one add them to the client array and listen to them from another thread there. 

Console.Write("Max Players: ");
maxPlayers = Int32.Parse(Console.ReadLine());
clients = new TcpClient[maxPlayers];
playerCount = 0;

formatter = new BinaryFormatter();
server = new TcpListener(IPAddress.Parse("192.168.1.33"), 7777);
server.Start();

while (true)
{
    if (server.Pending() && playerCount < maxPlayers)
    {
        Console.WriteLine("Found client");
        clients[playerCount] = server.AcceptTcpClient(); // Get client connection
        //When one player joins, this should start a thread with an a playerCount of 0
        Thread t = new Thread(() => ListenClient(playerCount));
        t.Start();
        playerCount++;
    }
}

public static void ListenClient(int index)
{
    while (true)
    {
        NetworkStream stream = clients[index].GetStream();
        object obj = formatter.Deserialize(stream);

        if (obj != null)
        {
            Console.WriteLine(obj);
        }
    }
}

However, when one player joins, the thread is called and passed an argument of 1, not 0 for some reason. What is the issue here?

RubixDude
  • 13
  • 2
  • Threading, my friend. It seems as if the reference to player-Count gets increased in the main thread, before Your second thread starts. Try to put a lock keyword around the increasing of the playercount. – icbytes Jul 15 '15 at 09:59
  • 2
    @icbytes this has nothing to do with threading, it is about how C# handles closures. – Andrey Jul 15 '15 at 10:01
  • Closure? How to detect it in here? – icbytes Jul 15 '15 at 10:01

4 Answers4

4

Well, that's how lambda works. Try this instead:

while (true)
{
    if (server.Pending() && playerCount < maxPlayers)
    {
        Console.WriteLine("Found client");
        clients[playerCount] = server.AcceptTcpClient(); // Get client connection
        //When one player joins, this should start a thread with an a playerCount of 0
        int currentPlayersCount = playerCount;
        Thread t = new Thread(() => ListenClient(currentPlayersCount));
        t.Start();
        playerCount++;
    }
}

EDIT:

As this was accepted as an answer, it should be mentioned that DavidG and Andrey's posts below give an additional important information about closures, and should be read to get the full picture.

shay__
  • 3,815
  • 17
  • 34
2

An alternative to shay__'s answer would be to use ParameterizedThreadStart.

Thread t = new Thread(new ParameterizedThreadStart((i) => ListenClient(i)));
t.Start(playerCount);
playerCount++;
Thorsten Dittmar
  • 55,956
  • 8
  • 91
  • 139
2

The issue you are seeing here is to do with closures. Take your code above, it passes in the variable playerCount, not the value of that variable. the subsequent lines run fast enough that the variable is incremented before the thread is started properly, hence the value is 1. To solve this, you can copy the value locally to the scope of the if block and pass that inside instead:

int localPlayerCount = playerCount;
Thread t = new Thread(() => ListenClient(localPlayerCount));

Further reading on closures here: What are 'closures' in .NET?

Community
  • 1
  • 1
DavidG
  • 113,891
  • 12
  • 217
  • 223
1

I will answer bigger question here, the practical part is answered by @shay__.

The problem here is incorrect usage of clojure. Closure (lambda in this case) doesn't just get a copy of variable value at the moment of creation, it gets reference to the variable, so when you actually get the value (call ListenClient you get value at that moment of time (which is 1). This is not very intuitive to the point that C# team made a breaking change and fixed it (or made more intuitive) in C# 5.0.

http://csharpindepth.com/Articles/Chapter5/Closures.aspx

Andrey
  • 59,039
  • 12
  • 119
  • 163
  • 1
    Pretty much what my answer said (though I referenced closures, not Clojure which is a programming language!) – DavidG Jul 15 '15 at 10:48
  • I must add something: If lambda + closures internally translates passed parameter value type to a class and therefore a reference type, then, WHY and FOR WHAT is this construct of closures effectively useful ? As Andrey says, this is not intuitive, and confuses more, then offering a reasonable possibility. In fact this ( if not being aware of accidently creating a closure ) really destroys all common senses about programming, doesn't it ? – icbytes Jul 24 '15 at 12:38
  • @icbytes no, passed parameters are treated as simple values nothing fancy. Closure is created only if you use local variables of outer function as if they were local variables of lambda (or anonymous methods. There are myriads of practical usages, Linq uses it heavily, for example '.Where(x => x > y)'. This lambda is closure because it captures y from outside. It is not intuitive but it works ok for most cases. There are really few cases where you can shoot yourself in the foot, like the one from question. But I would say that if you are messing with threads you should have solid understanding – Andrey Jul 24 '15 at 12:49
  • Your statement about messing with threads is right. But the closure just adds more complexity in this case, right ? – icbytes Jul 24 '15 at 12:50
  • @icbytes there are reasons why it works like that. Lambdas with closures are very convenient actually. They originally came from functional languages, where variables are immutable so it is always safe to pass them . Many imperative languages has this problem, Python as well for example. – Andrey Jul 24 '15 at 12:53
  • THX very much so far. – icbytes Jul 24 '15 at 12:54
  • @icbytes I am saying that if you are not doing anything advanced you are very unlikely to have troubles with closures. And if you want to do advanced stuff you should know everything you are doing :) . C# team actually removed one of the places where you can mess it up unintentionally http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/ – Andrey Jul 24 '15 at 12:57