Search code examples
c#multithreadingsocketstcpclient

Client not receiving data from Server Multithreading


I want to make a chat. The server is made in console app and the client is made in winforms.

In client I write a nickname and connect to server. The server receives name from client. I add all clients that connect to server in a Dictionary list with the (string)name and (TcpClient)Socket. After, I want to send to every client the client list.

When I debug on server, the Sockets appear with DualMode,EnableBroadcast error. In client when I have to receive the list it stops and doesn't do anything.

Server

namespace MyServer
{
    class MyServer
    {
        public Dictionary<string, TcpClient> clientList = new Dictionary<string, TcpClient>();
    TcpListener server = null;
    NetworkStream stream = null;
    StreamReader streamReader = null;
    StreamWriter streamWriter = null;
    TcpClient clientSocket;

    String messageReceived;
    int number_clients = 0;
    public MyServer(TcpClient clientSocket_connect)
    {
        stream = clientSocket_connect.GetStream();
        streamReader = new StreamReader(stream);
        streamWriter = new StreamWriter(stream);

        receiveMessage(clientSocket_connect); // receive messages
        }
        public MyServer()
        {
            Thread thread = new Thread(new ThreadStart(run));
            thread.Start();
        }           
        public void receiveMessage(TcpClient client_Socket)
        {
            messageReceived = streamReader.ReadLine();             

            if (messageReceived.Substring(messageReceived.Length - 4) == "user")
            {
                String name = messageReceived.Substring(0, messageReceived.Length - 4);
                bool found = false;
                foreach (var namefound in clientList.Keys)
                {
                    if (namefound == name)
                    {
                        found = true;
                        streamWriter.WriteLine("The user already exists");
                        streamWriter.Flush();
                    }
                }
                if (!found)
                {
                    //show who's connected
                    Console.WriteLine(name + " is online");
                    number_clients++;
                    clientList.Add(name, client_Socket);

                    //send to client clientlist
                    String send = null;
                    foreach (var key in clientList.Keys)
                    {
                        send += key + ".";
                    }
                    foreach (var value in clientList.Values)
                    {
                        TcpClient trimitereclientSocket = value;
                        if (trimitereclientSocket != null)
                        {
                            NetworkStream networkStream = trimitereclientSocket.GetStream();
                            StreamWriter networkWriter = new StreamWriter(networkStream);
                            networkWriter.WriteLine(send + "connected");
                            networkWriter.Flush();
                        }
                    }
                }
            }

        }
        void run()
        {
            IPAddress ipAddress = IPAddress.Parse("127.0.0.1");
            server = new TcpListener(ipAddress, 8000);
            server.Start();
            Console.WriteLine("Server started!");
            while (true)
            {
                clientSocket = server.AcceptTcpClient();                
                new MyServer(clientSocket);
            }
        }
    }
static void Main(string[] args)
        {
            MyServer server = new MyServer();
        }
}

Client

 namespace MyClient
    {
        class MyClient
        {
            List<string> clientList = new List<string>();

            TcpClient client = null;
            NetworkStream stream = nul

l;
        StreamReader streamReader = null;
        StreamWriter streamWriter = null;

        bool connected;
        String received_message;
        public MyClient()
        {
            client = new TcpClient("127.0.0.1", 8000);
            stream = client.GetStream();
            streamReader = new StreamReader(stream);
            streamWriter = new StreamWriter(stream);     
        }
        public void sendClientName(String name)
        {
            streamWriter.WriteLine(Convert.ToString(name));
            streamWriter.Flush();
        }
        public List<ClientName> receiveClientList()
        {
            List<ClientName> val = new List<ClientName>();         
                string name = Convert.ToString(streamReader.ReadLine());
                if (name.Substring(0, name.Length - 9) == "connected")
                {
                    ClientName client = new ClientName();
                    client.Nume = name;
                    val.Add(client);
                }          
            return val;
        }

    }
}

Client Form

 public partial class Form1 : Form
{
    MyClient client = new MyClient();
    public Form1()
    {
        InitializeComponent();
        Thread receiveClients = new Thread(new ThreadStart(getMessages));
    }

    private void btnConnect_Click(object sender, EventArgs e)
    {
        client.sendClientName(txtNickname.Text + "user");
    }
    public void getMessages()
    {
        while (true)
        {
            lbClientsConnected.Items.Add(client.receiveClientList());
        }
    }
}

Solution

  • I was unable to reproduce any error when running your code. I don't know what you mean by "the Sockets appear with DualMode,EnableBroadcast error". That said, there are a number of fixable problems with the code, including some that pertain directly to your concern that "when I have to receive the list it stops and doesn't do anything."

    Probably the biggest issue with the code is that you simply never start the client's receiving thread. You need to call the Start() method on the Thread object after it's been created:

    public Form1()
    {
        InitializeComponent();
        Thread receiveClients = new Thread(new ThreadStart(getMessages));
    
        // The receiving thread needs to be started
        receiveClients.Start();
    }
    

    Now, even with that fixed, you have a few other problems. The next big issue is that you are parsing the received text incorrectly. In your code, where you should be looking for the text "connected" at the end of the string, you instead extract the other part of the text (with the list of client names).

    Your receiveClientList() method should instead look like this:

    private const string _kconnected = "connected";
    
    public List<string> receiveClientList()
    {
        List<string> val = new List<string>();
        string name = Convert.ToString(streamReader.ReadLine());
    
        // Need to check the *end* of the string for "connected" text,
        // not the beginning.
        if (name.EndsWith(_kconnected))
        {
            name = name.Substring(0, name.Length - _kconnected.Length);
            val.Add(name);
        }
        return val;
    }
    

    (You didn't share the ClientName class in your question, and really the example doesn't need it; a simple string value suffices for the purpose of this exercise. ALso, I've introduced the const string named _kconnected, to ensure that the string literal is used correctly in each place it's needed, as well as to simplify usage.)

    But even with those two issues fixed, you've still got a couple in the Form code where you actually handle the return value of the receive method. First, you are passing the List<T> object that is returned from the receive method to the ListBox.Items.Add() method, which would just result in the ListBox displaying the type name for the object, rather than its elements.

    Second, because the code is executing in a thread other than the UI thread that owns the ListBox object, you must wrap the call in a call to Control.Invoke(). Otherwise, you'll get a cross-thread operation exception.

    Fixing those two issues, you get this:

    public void getMessages()
    {
        while (true)
        {
            // Need to receive the data, and the call Invoke() to add the
            // data to the ListBox. Also, if adding a List<T>, need to call
            // AddRange(), not Add().
            string[] receivedClientList = client.receiveClientList().ToArray();
    
            Invoke((MethodInvoker)(() => listBox1.Items.AddRange(receivedClientList)));
        }
    

    With those changes, the code will process the message sent by the client, and return the list of clients. That should get you further along. That said, you still have a number of other problems, including some fairly fundamental ones:

    1. The biggest issue is that when you accept a connection in the server, you create a whole new server object to handle that connection. There are a number of reasons this isn't a good idea, but the main one is that the rest of the code seems to conceptually assume that a single server object is tracking all of the clients, but each connection will result in its own collection of client objects, each collection having just one member (i.e. that client).

      Note that once you've fixed this issue, you will have multiple threads all accessing a single dictionary data structure. You will need to learn how to use the lock statement to ensure safe shared use of the dictionary across multiple threads.

    2. Another significant problem is that instead of using the streamWriter you created when you first accepted the connection, you create a whole new StreamWriter object (referenced in a local variable named networkWriter) to write to the socket. In this very simple example, it works fine, but between buffering and the lack of thread safety, this incorrectly-designed code could have serious data corruption problems.

    3. Less problematic, but worth fixing, is that your server code completely fails to take advantage of the fact that you're storing the clients in a dictionary, as well as that .NET has useful helper functions for doing things like joining a bunch of strings together. I would write your server's receiveMessage() method something more like this:

    private const string _kuser = "user";
    
    public void receiveMessage(TcpClient client_Socket)
    {
        messageReceived = streamReader.ReadLine();
    
        if (messageReceived.EndsWith(_kuser))
        {
            String name = messageReceived.Substring(0, messageReceived.Length - _kuser.Length);
    
            if (clientList.ContainsKey(name))
            {
                streamWriter.WriteLine("The user already exists");
                streamWriter.Flush();
                return;
            }
    
            //show who's connected
            Console.WriteLine(name + " is online");
            number_clients++;
            clientList.Add(name, client_Socket);
    
            string send = string.Join(".", clientList.Keys);
    
            foreach (var value in clientList.Values.Where(v => v != null))
            {
                // NOTE: I didn't change the problem noted in #2 above, instead just
                // left the code the way you had it, mostly. Of course, in a fully
                // corrected version of the code, your dictionary would contain not
                // just `TcpClient` objects, but some client-specific object specific
                // to your server implementation, in which the `TcpClient` object
                // is found, along with the `StreamReader` and `StreamWriter` objects
                // you've already created for that connection (and any other per-client
                // data that you need to track). Then you would write to that already-
                // existing `StreamWriter` object instead of creating a new one each
                // time here.
    
                NetworkStream networkStream = value.GetStream();
                StreamWriter networkWriter = new StreamWriter(networkStream);
                networkWriter.WriteLine(send + "connected");
                networkWriter.Flush();
            }
        }
    }
    

    The above is not exhaustive by any means. Frankly, you probably should spend more time looking at existing examples of network-aware code, e.g. on MSDN and Stack Overflow, as well as on tutorials on web sites, blogs, or in books. Even when you write the server in a one-thread-per-connection way as you seem to be trying to do here, there are lots of little details you really need to get correct, and which you haven't so far.

    But I do hope the above is enough to get you past your current hurdle, and on to the next big problem(s). :)