As Mark Gravell pointed out, this is a framing problem. Here is a simple client and server to show you how to frame your messages with a length prefix on the message. Keep in mind that this is just a sample to get you started. I would not consider it production ready code:
Client Code:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Sockets;
using System.Text;
namespace SimpleClient
{
internal class Client
{
private static void Main(string[] args)
{
try
{
TcpClient client = new TcpClient();
NetworkStream ns;
client.Connect("127.0.0.1", 560);
ns = client.GetStream();
byte[] buffer = ReadNBytes(ns, 4);
// read out the length field we know is there, because the server always sends it.
int msgLenth = BitConverter.ToInt32(buffer, 0);
buffer = ReadNBytes(ns, msgLenth);
//working with the buffer...
ASCIIEncoding encoder = new ASCIIEncoding();
string msg = encoder.GetString(buffer);
Console.WriteLine(msg);
client.Close();
}
catch
{
//displaying error...
}
}
public static byte[] ReadNBytes(NetworkStream stream, int n)
{
byte[] buffer = new byte[n];
int bytesRead = 0;
int chunk;
while (bytesRead < n)
{
chunk = stream.Read(buffer, (int) bytesRead, buffer.Length - (int) bytesRead);
if (chunk == 0)
{
// error out
throw new Exception("Unexpected disconnect");
}
bytesRead += chunk;
}
return buffer;
}
}
}
Server Code:
using System;
using System.Net;
using System.Net.Sockets;
using System.Text;
using System.Threading;
namespace SimpleServer
{
class Server
{
private static TcpListener tcpListener;
private static int clients;
static void Main(string[] args)
{
tcpListener = new TcpListener(IPAddress.Any, 560);
tcpListener.Start();
Console.WriteLine("Server started.");
while (true)
{
//blocks until a client has connected to the server
TcpClient client = tcpListener.AcceptTcpClient();
//create a thread to handle communication
//with connected client
Thread clientThread = new Thread(new ParameterizedThreadStart(HandleClientComm));
clientThread.Start(client);
}
}
private static void HandleClientComm(object client)
{
int clientCount = Interlocked.Increment(ref clients);
TcpClient tcpClient = (TcpClient)client;
NetworkStream clientStream = tcpClient.GetStream();
ASCIIEncoding encoder = new ASCIIEncoding();
Console.WriteLine("Client connected. ({0} connected)", clientCount);
#region sendingHandler
byte[] buffer = encoder.GetBytes("Some Contacts as a string!");
byte[] lengthBuffer = BitConverter.GetBytes(buffer.Length);
clientStream.Write(lengthBuffer, 0, lengthBuffer.Length);
clientStream.Write(buffer, 0, buffer.Length);
clientStream.Flush();
tcpClient.Close();
#endregion
}
}
}
The reason your code sometimes worked, and sometimes failed is that client.Available can return 0. When it did you were setting the bytes to read to 32k, so the read call was waiting for those bytes to come in. They never did, and since the server never closed the socket, read would not error out either.
Hope this gets you moving in the right direction.
Edit:
I forgot to mention endianess in my original post. You can see the documentation here about endianess and using BitConverter: http://msdn.microsoft.com/en-us/library/system.bitconverter(v=vs.100).aspx
Basically you need to make sure both server and client are running on architectures with the same endianess, or handle the conversion from one endianess to another as needed.
Edit 2 (to answer question in the comments):
1) Why can client.available return 0?
This is a timing issue. The client is connecting to the server, then immediately asking which bytes are available. Depending on what other processes are running, time slices for the available processor etc, the client may be asking what is available before the server has had a chance to send anything at all. In this case it will return 0.
2) Why did I use Interlocked to increment the clients?
The code as you had originally written it was incrementing the clients in the newly created thread running HandleClientComm(...). If two or more clients connected simultaneously it is possible a race condition could occur as multiple threads were trying to increment clients. The end result would be clients would be less than it should be.
3) Why did I change ReadFully method?
You version of ReadFully, which I changed to ReadNBytes, was close to be correct, but had a few flaws:
- Setting initialLenth to 32768 if the original initialLength was zero or less. You should never guess how many bytes you need to read from a socket. As Mark Gravell mentioned you need to frame your messages with either a length prefix or some sort of delimiter.
- NetworkStream.Read blocks until some bytes are read or returns a 0 if the socket is closed out from underneath it. There was no need to call stream.ReadByte, since chunk would already be 0 if the socket was disconnected. Making that change simplified the method, especially since we know exactly how many bytes we need to read based on our simple header.
- Now that we know how much we are going to read, we can allocate exactly what we need up front. This removes the necessity of re-sizing the buffer upon return. We can just return what we allocated.
4) How do I know the length buffer size is 4?
The length I serialized was 32 bits. You can see that in the documentation for BitConverter.GetBytes(int value). We know a byte is 8 bits, so simply divide 32 by 8 giving us 4.
5) Why is this not production ready/How can I improve the code?
Basically there is no real error handling. NetworkStream.Read() can throw several exceptions, none of which I am handling. Adding the proper error handling would go a long way to making it production ready.
I have not really tested the code beyond a cursory run. It would need to be tested under a variety of conditions.
There is no provision for the client to reconnect or retry, though you may not need this for your purposes.
This was written as a simple example, and may not actually meet the requirements you are trying to fulfill. Not knowing those requirements I cannot claim this is ready for your production environment (whatever that is).
Conceptually ReadNBytes is fine, but if someone sends you malicious message which claim the length of the message is 2 gigabytes or something, you are going to try and allocate 2 gigabytes blindly. Most byte level protocols (the description of what goes over the wire) specify the maximum size of messages. That would need to be checked, and if the messages can actually be large you would need to handle it differently than just allocating a buffer, maybe writing to a file or other output stream as you read it in. Again, not knowing your full requirements, I cannot be sure what is needed there.
Hope this helps.
