every time I close a client, the server stops working. I get an error message, that it is "unable to read beyond the end of the stream"
This is, in some sense, completely normal. That is, when using BinaryReader, its normal behavior is to throw EndOfStreamException when reaching the end-of-stream.
Why did it reach end-of-stream? Well, because the client disconnected and that's what happens to the stream. At the socket level, what really happens is that a read operation completes with 0 as the count of bytes read. This is the indication that the client has gracefully closed the socket and will not be sending any more data.
In the .NET API, this is translated to the end of the NetworkStream that TcpClient uses to wrap the Socket object that's actually handling the network I/O. And this NetworkStream object is in turn being wrapped by your BinaryReader object. And BinaryReader throws that exception when it reaches the end of the stream.
Note that your code does not actually provide a graceful way for the user to close a client. They will have to use Ctrl+C, or kill the process outright. Using the former has the serendipitous effect of performing a graceful shutdown of the socket, but only because .NET is handling the termination of the process and runs finalizers on your objects, such as the TcpClient object used to connect to the server, and the finalizer calls Socket.Shutdown() to tell the server it's closing.
If you were to kill the process (e.g. using Task Manager), you'd find an IOException was thrown instead. Good network code should always be prepared to see an IOException; networks are unreliable and failures do occur. You want to do some reasonable, such as removing the remote endpoint from your connections, rather than just having the whole program crash.
Now, all that said, just because the EndOfStreamException is "normal", that does not mean the code you posted is, or is in any way an example of the right way to do network programming. You have a number of issues:
- There is no explicit graceful closure.
Network I/O provides a normal way to close connections, which involves handshaking on both endpoints to indicate when they are done sending, and when they are done receiving. One endpoint will indicate it's done sending; the other will note this (using the 0-byte-read mentioned above) and then itself indicate it's done both sending and receiving.
TcpClient and NetworkStream don't expose this directly, but you can use the TcpClient.Client property to get the Socket object to do a better graceful closure, i.e. one endpoint can indicate it's done sending, and still be able to wait until the other endpoint is also done sending.
Using the TcpClient.Close() method to disconnect is like hanging up the phone on someone without saying "good-bye". Using Socket.Shutdown() is like finishing a phone call with a polite "okay, that's everything I wanted to say…was there anything else?"
- You are using
BinaryReader but not handling the EndOfStreamException correctly.
- Your client uses two connections to communicate with the server.
Network I/O uses the Socket object, which supports full-duplex communications. There is no need to create a second connection just to do both reading and writing. A single connection suffices, and is better because when you split the send and receive into two connections, then you also need to add something to your protocol so that the server knows those two connections represent a single client (which your code does not actually do).
- A client is not removed from server list when it disconnected (you noted this in your question).
- The client list is not thread-safe.
- Your
Chat() method has an extra "while (true)" in it.
I have modified your original example to address all of the above, which I've presented here:
Server Program.cs:
class Program
{
private static readonly object _lock = new object();
private static readonly List<TcpClient> clients = new List<TcpClient>();
public static TcpClient[] GetClients()
{
lock (_lock) return clients.ToArray();
}
public static int GetClientCount()
{
lock (_lock) return clients.Count;
}
public static void RemoveClient(TcpClient client)
{
lock (_lock) clients.Remove(client);
}
static void Main(string[] args)
{
IPAddress ip = IPAddress.Parse("127.0.0.1");
TcpListener ServerSocket = new TcpListener(ip, 14000);
ServerSocket.Start();
Console.WriteLine("Server started.");
while (true)
{
TcpClient clientSocket = ServerSocket.AcceptTcpClient();
Console.WriteLine($"client connected: {clientSocket.Client.RemoteEndPoint}");
lock (_lock) clients.Add(clientSocket);
handleClient client = new handleClient();
client.startClient(clientSocket);
Console.WriteLine($"{GetClientCount()} clients connected");
}
}
}
Server handleClient.cs:
public class handleClient
{
TcpClient clientSocket;
public void startClient(TcpClient inClientSocket)
{
this.clientSocket = inClientSocket;
Thread ctThread = new Thread(Chat);
ctThread.Start();
}
private void Chat()
{
BinaryReader reader = new BinaryReader(clientSocket.GetStream());
try
{
while (true)
{
string message = reader.ReadString();
foreach (var client in Program.GetClients())
{
BinaryWriter writer = new BinaryWriter(client.GetStream());
writer.Write(message);
}
}
}
catch (EndOfStreamException)
{
Console.WriteLine($"client disconnecting: {clientSocket.Client.RemoteEndPoint}");
clientSocket.Client.Shutdown(SocketShutdown.Both);
}
catch (IOException e)
{
Console.WriteLine($"IOException reading from {clientSocket.Client.RemoteEndPoint}: {e.Message}");
}
clientSocket.Close();
Program.RemoveClient(clientSocket);
Console.WriteLine($"{Program.GetClientCount()} clients connected");
}
}
Client Program.cs:
class Program
{
private static readonly object _lock = new object();
private static bool _closed;
public static void Write(TcpClient client)
{
try
{
string str;
SocketShutdown reason = SocketShutdown.Send;
while ((str = Console.ReadLine()) != "")
{
lock (_lock)
{
BinaryWriter writer = new BinaryWriter(client.GetStream());
writer.Write(str);
if (_closed)
{
// Remote endpoint already said they are done sending,
// so we're done with both sending and receiving.
reason = SocketShutdown.Both;
break;
}
}
}
client.Client.Shutdown(reason);
}
catch (IOException e)
{
Console.WriteLine($"IOException writing to socket: {e.Message}");
}
}
public static void Read(TcpClient client)
{
try
{
while (true)
{
try
{
BinaryReader reader = new BinaryReader(client.GetStream());
Console.WriteLine(reader.ReadString());
}
catch (EndOfStreamException)
{
lock (_lock)
{
_closed = true;
return;
}
}
}
}
catch (IOException e)
{
Console.WriteLine($"IOException reading from socket: {e.Message}");
}
}
static void Main(string[] args)
{
TcpClient client = new TcpClient("127.0.0.1", 14000);
Thread writeThread = new Thread(() => Write(client));
Thread readThread = new Thread(() => Read(client));
writeThread.Start();
readThread.Start();
writeThread.Join();
readThread.Join();
client.Close();
Console.WriteLine("client exiting");
}
}
Note that I did not, for the most part, address the inconsistent and unconventional naming you've used in your code. The only exception was for the thread variables in the client code, because I really don't like capitalized local variables that exactly match the name of a type.
You have some other problems as well, which the above revision of your code does not address. These include:
- You're using
BinaryReader. This is in a lot of ways an annoying class to use. I recommend, especially for a chat-server scenario where you're only dealing with text anyway, that you switch to using StreamReader/StreamWriter.
- There is improper coupling/separation of concerns. Your
Program class has server code, and the server code knows about the Program class. It would be much better to encapsulate both the server and client implementations into their own classes, separate from the main entry-point of the program, and to further decouple the top-level server code with the per-client data structure (use C#'s event to allow the top-level server code to be notified of important events, such as the need to remove a client from the list, without having the per-client data structure have to actually know about the top-level server object, never mind its client list).
- You should provide a mechanism to gracefully shutdown server.
Normally, I would say that these are outside the scope of an answer like this, which is already quite long. I've addressed the immediate concern in your code and then some, and that is nominally sufficient.
However, I've been meaning to write an updated version of the basic network programming example I'd written some years ago, as a sort of "intermediate" example, adding multiple client support, asynchronous operation, and using the latest C# features (like async/await). So, I went ahead and took some time to do that. I guess I'll post it to my blog eventually…that's a whole other project. In the meantime, here's that code (note that this is an entirely from-scratch example…it made more sense to do that than to try to rework the code you had)…
Most of the grunt work for this implementation is in a single class shared by the server and client:
/// <summary>
/// Represents a remote end-point for the chat server and clients
/// </summary>
public sealed class ConnectedEndPoint : IDisposable
{
priva
