3
\$\begingroup\$

I'm just learning how to allow interconnected apps via Sockets, watched a tutorial last night and based on that this is what I've gotten:

Server.cs

Code:

using System;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Threading;

namespace SocketsLiveTest
{
    internal class Server
    {
        private static void Main(string[] args)
        {
            //listener to handle incoming connections
            TcpListener listener = new TcpListener(IPAddress.Any, <redacted port>);
            listener.Start();
            //log that it started
            Console.WriteLine("Server started, waiting for connections...");

            TcpClient client;

            while (true)
            {
                //connect the client
                client = listener.AcceptTcpClient();
                //log client connected
                Console.WriteLine("Client connected @ {0} - {1}", ((IPEndPoint)client.Client.RemoteEndPoint).Address.ToString(), DateTime.Now.ToString());
                
                //send to another thread to allow other connections, probably won't be necessary as there should only be 1 client
                ThreadPool.QueueUserWorkItem(DoClientWork, client);
            }
        }

        private static void DoClientWork(object obj)
        {
            //get client
            var client = (TcpClient)obj;
            //NetworkStream clientStream = client.GetStream();
            
            //initialize stream writer to send data to client
            StreamWriter clientWriter = new StreamWriter(client.GetStream());
            clientWriter.AutoFlush = true; //for testing purposes, will probably manually flush in final

            int breakAmount = 1; //prevent looping during testing

            while (true)
            {
                //junk message for testing
                clientWriter.WriteLine("DID THIS WORK? Push {0}", breakAmount);

                //more testing stuff
                breakAmount++;
                if (breakAmount >= 10)
                    break;
            }
        }
    }
}

The breakAmount portion of DoClientWork is purely to prevent a loop as currently I haven't thought of a close reason. This will be used in some sort of pub/sub service eventually to push some serialized objects over to the client. I'm thinking I will just have it infinitely loop and check a database for new info to send, since this should really always be running unless manually shutdown. Would that be the appropriate way to handle a pub/sub relationship? Get them connected and then continuously loop for data to queue up and send to the client?

This is a client I wrote to just test functionality:

Client.cs

Code:

using System;
using System.IO;
using System.Net.Sockets;

namespace SocketsClientLiveTest
{
    internal class Client
    {
        private static void Main(string[] args)
        {
            int connectionAttempts = 1;
            int allowedAttempts = 10;

            bool connectionSuccess = false;

            TcpClient client = new TcpClient();

            while (connectionAttempts <= allowedAttempts)
            {
                Console.WriteLine("Attempting connection, attempt: {0} of {1}", connectionAttempts.ToString(), allowedAttempts.ToString());

                try
                {
                    client = new TcpClient("192.168.15.32", 4040);
                    Console.WriteLine("Successful connection on attempt {0}", connectionAttempts);
                    Console.WriteLine("Attempting to read...");

                    connectionSuccess = true;
                    break;
                }
                catch (Exception)
                {
                    connectionAttempts++;
                    Console.WriteLine("Failed to connect, retrying...");
                }
            }

            if (connectionSuccess)
            {
                bool doneReading = false;
                int readAttempts = 1;
                while (!doneReading)
                {
                    StreamReader serverReader = new StreamReader(client.GetStream());
                    string serverMessage = serverReader.ReadLine();
                    Console.WriteLine("Server message received: {0}", serverMessage);

                    readAttempts++;
                    if (readAttempts > 3)
                    {
                        doneReading = true;
                    }
                }
            }
        }
    }
}

I'm not entirely concerned about Client.cs since, as I said, I'm not going to be responsible for it. If you have ideas on it though, I'd love to hear them for educational purposes, but like the server breakAmount, it does just have some junk in there to prevent looping forever.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ After reading the title, I thought you were trying to implement some sort of artificial intelligence machine learning socket ;-) \$\endgroup\$ Commented Nov 17, 2021 at 17:37
  • \$\begingroup\$ Is there a reason why you declare client outside of the server loop instead of using var client = listener.AcceptTcpClient(); or TcpClient client = listener.AcceptTcpClient();? \$\endgroup\$ Commented Nov 18, 2021 at 3:47

1 Answer 1

Reset to default
4
\$\begingroup\$

Disposing of Disposables

The main problem in your code is that you are not disposing of clientWriter. A good rule to remember is that you should always dispose of disposable objects. If you do not dispose of clientWriter then it will never be flushed or closed and will result in a bucket of hurt.

Consider using using to automatically dispose of clientWriter

using StreamWriter clientWriter = new StreamWriter(client.GetStream());
clientWriter.AutoFlush = true;

Infinite Loops

In DoClientWork you have an infinite loop and break out when breakAmount is >= 10

Consider changing it to the following:

while (breakAmount < 10)

Or, even better, use a for loop:

for (int i=1; i <= 10; i++)
{
    //junk message for testing
    clientWriter.WriteLine("DID THIS WORK? Push {0}", i);
}

Reading all the text in the stream

In the client code, you are simply repeating calls to serverReader.ReadLine instead of checking if there is anything left in the stream. Consider changing to the following:

if (connectionSuccess)
{
    StreamReader serverReader = new StreamReader(client.GetStream());
    while (serverReader.EndOfStream == false)
    {
        Console.WriteLine(serverReader.ReadLine());
    }
}

Two-way communication

You can easily add two-way communication by using the following code

Server

private static void DoClientWork(object obj)
{
    using var client = (TcpClient)obj;
    using var writer = new StreamWriter(client.GetStream());
    using var reader = new StreamReader(client.GetStream());
    for (int i=1; i <= 10; i++)
    {
        writer.WriteLine("DID THIS WORK? Push {0}", i);
        writer.Flush();
        // read response
        string response = reader.ReadLine();
        Console.WriteLine(response);
    }
}

Client

if (connectionSuccess)
{
    using var reader = new StreamReader(client.GetStream());
    using var writer = new StreamWriter(client.GetStream());
    while (reader.EndOfStream == false)
    {
        string line = reader.ReadLine();
        Console.WriteLine(line);
        // write response
        writer.WriteLine(line.Replace("DID THIS WORK?", "YES IT DID!"));
        writer.Flush();
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ @Austin, for older versions of C# you need to use curly braces to define the scope of using like the this. Ps I had some fun playing with sockets. \$\endgroup\$ Commented Nov 17, 2021 at 21:19
  • 1
    \$\begingroup\$ @Austin Cool I get that =) The thing is that we are not reviewing you as a programmer, but only the code in question. Some of the observations and tips may be things that you know but may be helpful to other people purely looking at the code. \$\endgroup\$ Commented Nov 18, 2021 at 0:03
  • 1
    \$\begingroup\$ You'll probably also want to dispose of the client in the DoClientWork function (using TcpClient client = (TcpClient)obj; or similar). \$\endgroup\$ Commented Nov 18, 2021 at 3:45
  • \$\begingroup\$ @user, I think you're correct although it looks a bit odd to dispose of something that you are technically not the owner of? \$\endgroup\$ Commented Nov 18, 2021 at 12:29

You must log in to answer this question.