Tcp client server form app issue

Question

I have stuck with a problem and that is when I try to write from my server to my multiple clients, it keeps on writing the empty string. I'm sharing my code and its multiple client server app in which clients can write to server while server can write to all running clients. Clients writing to server quiet fine. but server isn't :(

Server code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Net;
using System.IO;
using System.Net.Sockets;
using System.Threading;
using System.Collections;

namespace ServerGui
{
    public partial class Form1 : Form
    {
        TcpListener tcpListen;
        Thread listenThread;
        String read = "";
        ArrayList collect = new ArrayList();
        int counter = 0;
        ArrayList NS = new ArrayList();
        TcpClient general = null;
        readonly ManualResetEvent mre = new ManualResetEvent(false);

        public Form1()
        {
            InitializeComponent();
        }

        private void connectServerToolStripMenuItem_Click(object sender, EventArgs e)
        {
            tcpListen = new TcpListener(IPAddress.Any, 3000);
            listenThread = new Thread(new ThreadStart(ListenForClients));
            listenThread.Start();
            connectServerToolStripMenuItem.Enabled = false;
        }

        public void ListenForClients()
        {
            tcpListen.Start();

            while (true)
            {

                TcpClient client = tcpListen.AcceptTcpClient();
                collect.Insert(counter, client);
                general = (TcpClient)collect[counter];
                if (textBox1.InvokeRequired)
                {
                    textBox1.Invoke(new MethodInvoker(delegate { textBox1.AppendText(client.Client.LocalEndPoint.ToString() + "  Connected.");
                    textBox1.AppendText(Environment.NewLine); }));
                }
                else
                {
                    textBox1.AppendText(client.Client.LocalEndPoint.ToString() + "  Connected.");
                    textBox1.AppendText(Environment.NewLine);
                }
                Thread clientThread = new Thread(new ParameterizedThreadStart(Reader));
                clientThread.Start(collect[counter]);
                Thread clientThread1 = new Thread(new ParameterizedThreadStart(Writer));
                clientThread1.Start(collect[counter]);
                counter++;
            }
        }
        public void Reader(object client)
        {
            TcpClient tcpClient = (TcpClient)client;
            NetworkStream clientStream = tcpClient.GetStream();
            StreamReader sr = new StreamReader(clientStream);
            while (true)
            {
                try
                {
                    read = sr.ReadLine();
                }
                catch
                {
                    if (textBox1.InvokeRequired)
                    {
                        textBox1.Invoke(new MethodInvoker(delegate
                        {
                              textBox1.AppendText(tcpClient.Client.LocalEndPoint.ToString() +
 " disconnected.");
                            textBox1.AppendText(Environment.NewLine);
                        }));
                    }
                    else
                    {
                        textBox1.AppendText(tcpClient.Client.LocalEndPoint.ToString() + " disconnected.");
                        textBox1.AppendText(Environment.NewLine);
                    }
                    return;
                }
                if (textBox1.InvokeRequired)
                {
                    textBox1.Invoke(new MethodInvoker(delegate
                    {
                        textBox1.AppendText("<" +         tcpClient.Client.LocalEndPoint.ToString() + "> saying: " + read);
                        textBox1.AppendText(Environment.NewLine);
                    }));
                }
                else 
                {
                    textBox1.AppendText("<" + tcpClient.Client.LocalEndPoint.ToString() + "> saying: " + read);
                    textBox1.AppendText(Environment.NewLine);
                }
            }

            tcpClient.Close();
        }

        public void Writer(object client)
        {
            TcpClient tcpClient = (TcpClient)client;
            NetworkStream ns = tcpClient.GetStream();
            mre.WaitOne();
            while (true)
            {
                StreamWriter sw = new StreamWriter(ns);

                    string str = textBox2.Text;
                    sw.WriteLine(str);
                    sw.Flush();

                if (textBox2.InvokeRequired)
                {
                    textBox2.Invoke(new MethodInvoker(delegate 
                        {
                            textBox2.Clear();
                        }));
                }
                else
                textBox2.Clear();
                }
            tcpClient.Close();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            mre.Set();
            textBox1.AppendText(textBox2.Text);
            textBox1.AppendText(Environment.NewLine);
            textBox2.Clear();
        }

        private void CheckKeys(object sender, System.Windows.Forms.KeyPressEventArgs e)
        {
            if (e.KeyChar == (char)13)
            {
                mre.Set();
                textBox1.AppendText(textBox2.Text);
                textBox1.AppendText(Environment.NewLine);
                textBox2.Clear();
            }
        }

        private void exitToolStripMenuItem_Click(object sender, EventArgs e)
        {
            Application.Exit();
        }
    }
}

Client code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Net;
using System.Net.Sockets;
using System.Threading;
using System.IO;

namespace ClientGcui
{
    public partial class Form1 : Form
    {
        NetworkStream general = null;
        public Form1()
        {
            InitializeComponent();

        }

        private void exitToolStripMenuItem_Click(object sender, EventArgs e)
        {
            Application.Exit();
        }

        private void connectToServerToolStripMenuItem_Click(object sender, EventArgs e)
        {
            String str = "";
            TcpClient client = new TcpClient();
            IPEndPoint server = new IPEndPoint(IPAddress.Parse("127.0.0.1"), 3000);
            client.Connect(server);
            textBox1.AppendText("Connected to server");
            textBox1.AppendText(Environment.NewLine);
            NetworkStream clientStream = client.GetStream();
            general = clientStream;
            StreamReader sr = new StreamReader(clientStream);
            StreamWriter sw = new StreamWriter(clientStream);
            Thread reader = new Thread(new ParameterizedThreadStart(readserver));
            reader.Start(sr);
        }
        public void readserver(object client)
        {
            StreamReader sr = (StreamReader)client;
            string read = "";
            while (true)
            {
                try
                {
                    read = sr.ReadLine();
                }
                catch
                {
                    if (textBox1.InvokeRequired)
                    {
                        textBox1.Invoke(new MethodInvoker(delegate
                        {
                            textBox1.AppendText("Disconnected from Server.");
                            textBox1.AppendText(Environment.NewLine);
                        }));
                    }
                    else
                    {
                        textBox1.AppendText("Disconnected from Server.");
                        textBox1.AppendText(Environment.NewLine);
                    }
                }
                if (textBox1.InvokeRequired)
                {
                    textBox1.Invoke(new MethodInvoker(delegate
                    {
                        textBox1.AppendText(sr.ReadLine());
                        textBox1.AppendText(Environment.NewLine);
                    }));
                }
                else
                {
                    textBox1.AppendText(sr.ReadLine());
                    textBox1.AppendText(Environment.NewLine);
                }
                }
            sr.Close();
            }


        private void button1_Click(object sender, EventArgs e)
        {
            StreamWriter sw = new StreamWriter(general);
            sw.WriteLine(textBox2.Text);
            sw.Flush();
            textBox1.AppendText(textBox2.Text);
            textBox1.AppendText(Environment.NewLine);
            textBox2.Clear();
        }

        private void CheckKeys(object sender, System.Windows.Forms.KeyPressEventArgs e)
        {
            if (e.KeyChar == (char)13)
            {
                StreamWriter sw = new StreamWriter(general);
                sw.WriteLine(textBox2.Text);
                sw.Flush();
                textBox1.AppendText(textBox2.Text);
                textBox1.AppendText(Environment.NewLine);
                textBox2.Clear();
            }
        }
    }
}

Answer 1

What's with 'textBox1', 'textbox2', 'button1'. There dosn't seem to be one application-specific component name in this code - you know, the stuff that makes it easily and quickly understandable to experienced developers quickly looking through posts. Why have you not edited the component names to something meaningful before posting here? It's not like your English is bad, (which I could then understand), your question is clear enough.

Anyway, your writer is not Invoke()ing the textbox2 read and then the first writer to clear textBox2 wins the race and stuffs up all the other writers who then read nothing.

..and in the CheckKeys your are signalling the mre and then running on an clearing the textbox2 before the race-winning writer even gets a chance to read it.

I can't see where you reset the mre which, anyway, is the wrong synchro object to use.

You cannot skimp on multithreaded code. You have to do it correctly.

There are some possibilities for comms with your writer thread.

1) Use a lock-protected, reference-counted object to hold on to your outgoing text from textbox2 until all the writers have done with it. This implies that you know how many writers there are - something you are no keeping track of, at present. The problem with this is that writers must not exit without deccing the refCount of all 'hold' instances whose refCount was initialized to include. I doubt that you will get this right.

2) Use an object to hold on to your outgoing text from textbox2. Also, submit this instance to a timeout queue that holds onto a reference for, say, 10 minutes, by which time all writers will definitely have used it. This is a reasonable approach.

3) Use an object to hold on to your outgoing text from textbox2 for each writer and submit a different copy on a producer-consumer queue to each writer. This means that you need a vector of writers that must be kept up-to-date as clients connect and disconnect.

4) There was a fourth way that I thought of when writing the other three, but I've forgot it now.

I would go with (2) - least chance of something not working.

Oh - forgot, signalling all the client threads. If you use an MRE, where are you going to reset it? How can you tell when all clients have read stuff and are about to wait again? You can't, and I suspect that this is why you have no resetEvent in your code - you don't know where to put it.

Thining about it, given that you have a GC language, it might be simplest and safest to give each writer its own BlockingCollection and queue the reference to each text object to all writers. Again, we are back to a, (thread-safe), collection of writers that have new entries put in on connect and removed on disconnect. Even with the thread-safe collection, you must expect, and catch, the odd exception because the main thread tries to issue a text object ref. to a writer that has just disconnected by not quite got round to removing itself from the collection yet.

Also, a thread per writer is a bit of overkill. For max. clients, you would be better off using the threadPool for writes. You would only then need one 'SeverClientSocket' class with would have its own read thread, a queueUpForWrite() method for the main thread to add to and a writeQueue() method for a pool thread to call.