1
\$\begingroup\$

I have a simple application that can receive and send tcp data. right now there's a thread used to start the listener and within this thread there's another thread to receive.

it works but it seems like a waste of resources. Can the connect be simplified?

    public partial class Form1 : Form
    {
    private readonly TcpListener server = new TcpListener(IPAddress.Any, 1980);
    private NetworkStream sendStream;
    private NetworkStream recvStream;
    private TcpClient client;
    private byte[] datalength = new byte[4];

    public Form1()
    {
        InitializeComponent();
    }

    public void ServerReceive()
    {
        recvStream = client.GetStream();
        new Thread(() =>
        {
            while (recvStream.Read(datalength, 0, 4) != 0)
            {
                byte[] data = new byte[BitConverter.ToInt32(datalength, 0)];
                recvStream.Read(data, 0, data.Length);
                this.Invoke((MethodInvoker)delegate
                {
                    txtLog.Text += System.Environment.NewLine + "Client : " + Encoding.Default.GetString(data);
                });
            }
        }).Start();
    }

    public void ServerSend(string msg)
    {
        sendStream = client.GetStream();
        var data = Encoding.Default.GetBytes(msg);
        var length = data.Length;
        datalength = BitConverter.GetBytes(length);
        sendStream.Write(datalength, 0, 4);
        sendStream.Write(data, 0, data.Length);
    }

    private void btnListen_Click(object sender, EventArgs e)
    {
        Connect();
    }

    private void btnSend_Click(object sender, EventArgs e)
    {
        if (client.Connected)
        {
            ServerSend(txtSend.Text);
        }
    }

    private void Form1_Load(object sender, EventArgs e)
    {
        Connect();
    }

    private void Connect()
    {
        server.Start();
        MessageBox.Show("Waiting For Connection");
        new Thread(() =>
        {
            client = server.AcceptTcpClient();
            MessageBox.Show("Connected To Client");
            if (client.Connected)
            {
                ServerReceive();
            }
        }).Start();
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I'm not knowledgable enough in this but you can use the async methods (BeginAccept, BeginRead et al) \$\endgroup\$ Nov 22 '16 at 10:05
1
\$\begingroup\$

Various things about your code seem odd.

Field level values

There's no reason for these variables to be declared at a class level, you only ever use them within a single function:

private NetworkStream sendStream;
private NetworkStream recvStream;

btnSend_Click bug

If the user clicks on the send button, before they've clicked on the Listen button, your client member won't have been initialised so will throw an exception when you do:

if (client.Connected)

Listen/Read Thread

It looks like you're only expecting a single client (your listen thread doesn't contain a loop to accept more than a single connection). Rather than spinning up another thread to perform the receive functionality, you might as well be doing it in the same thread that you performed the listen on.

Listen Management

Your listen code doesn't check the state of the Server before calling start. If you click it twice, it seems likely that this is going to cause a problem.

Exception Handling

You don't have any exception handling around your read/write calls. TCP can be a bit of a pain at times, depending upon how the connection has been shutdown it may throw various exceptions rather than just returning 0 from the read call. It's also worth remembering that since you're accessing the socket from multiple threads it can change state between your is connected call and your write operation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.