Learning Sockets in C# (.Net Framework), Suggested Improvements?

Question

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.

• jdt: After reading the title, I thought you were trying to implement some sort of artificial intelligence machine learning socket ;-)
• user: 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();?

Answer 1

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();
 }
}

• jdt: @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.
• jdt: @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.
• user: You'll probably also want to dispose of the client in the DoClientWork function (using TcpClient client = (TcpClient)obj; or similar).
• jdt: @user, I think you're correct although it looks a bit odd to dispose of something that you are technically not the owner of?