C# socket receiving incomplete data

Question

I'm working on a server/client pair of apps which work together as a remote file explorer, like Filezilla.
TCP sockets are used for communication, and this is the central class both of them use to handle communication:

public class SocketHelper
{
    public static async Task SendAsync(Socket socket, object message)
    {
        // Size
        var sendBuffer = Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(message));
        await socket.SendAsync(BitConverter.GetBytes(sendBuffer.Length));

        // Payload
        var sentBytes = 0;

        while (sentBytes < sendBuffer.Length)
        {
            sentBytes += await socket.SendAsync(sendBuffer);
        }
    }

    public static async Task<string> ReceiveAsync(Socket socket)
    {
        // Size
        var preflightBuffer = new byte[32];
        await socket.ReceiveAsync(preflightBuffer);

        // Payload
        var receiveBuffer = new byte[BitConverter.ToInt32(preflightBuffer)];
        var receivedBytes = 0;

        while (receivedBytes < receiveBuffer.Length)
        {
            receivedBytes += await socket.ReceiveAsync(receiveBuffer);
        }

        return Encoding.UTF8.GetString(receiveBuffer);
    }
}

As you can see I'm sending the size first, then counting the amount of bytes sent and received every time in order to coordinate and limit how much I'm sending and receiving.
This works perfectly fine for small 'messages', such as simple serialized requests/response objects for directory listings like these:

public class FolderListingRequest
{
    public string Path { get; set; }
}

public class FolderListingResponse : Response
{
    public IList<FileData> Files { get; set; } = [];
}

However for larger amounts of data (file download) it falls apart because somewhere during the transfer the messages come in either incomplete or with extra data at the end, making the deserialization fail.
Furthermore, it's completely inconsistent in when it fails. Sometimes sending 5 files in one go works fine yet sometimes it breaks with one single file.

Here's the server code which requests and handles file download:

public async Task DownloadFiles(Socket socket, DownloadFilesRequest request)
{
    var payload = new Payload
    {
        Type = PayloadTypeEnum.DownloadFiles,
        Data = request,
    };
    var fileCountToReceive = request.Paths.Count;
    var fileCountReceived = 0;
    await SocketHelper.SendAsync(socket, payload);

    while (fileCountReceived < fileCountToReceive)
    {
        Console.WriteLine("Receiving file");
        var receivedData = await SocketHelper.ReceiveAsync(socket);
        var fileData = JsonConvert.DeserializeObject<FileData>(receivedData);

        if (fileData == null)
        {
            Console.WriteLine("Could not deserialize downloaded file info");
        }

        localFileService.StoreFile(fileData);
        fileCountReceived++;
    }
}

And here's the client's part which sends the files:

public async Task SendFiles(Socket socket, DownloadFilesRequest request)
{
    foreach (var path in request.Paths)
    {
        var response = new FileData()
        {
            Path = path,
            Contents = localFileService.GetFileContents(path),
        };

        await SocketHelper.SendAsync(socket, response);
    }
}

Is it possible that this is some sort of synchronization issue where the client sends more data than the server is designed to receive? Shouldn't counting the bytes be enough to alleviate this? Should I be using markers for the beginning and ending of the messages instead? Or am I trying to send too much data at a time? Should I chunk it?

Any thoughts?

Answer 1

You have a couple of mistakes here

• preflightBuffer is the wrong size in the recieve, it should be 4 bytes.
• The read and write loops need to pass an offset.

Rather than messing around with raw sockets and read loops, you can use TcpClient to get a NetworkStream over a TCP socket, then you can just use ReadExactlyAsync.

public static async Task SendAsync(Stream stream, object message)
{
    // Size
    var sendBuffer = Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(message));
    await stream.SendAsync(BitConverter.GetBytes(sendBuffer.Length));

    // Payload
    await stream.WriteAsync(sendBuffer);
}

public static async Task<string> ReceiveAsync(Stream stream)
{
    // Size
    var preflightBuffer = new byte[4];
    await stream.ReadExactlyAsync(preflightBuffer);

    // Payload
    var receiveBuffer = new byte[BitConverter.ToInt32(preflightBuffer)];
    await stream.ReadExactlyAsync(receiveBuffer);
    return Encoding.UTF8.GetString(receiveBuffer);
}

You can make this more efficient by using ArrayPool and by changing over to System.Text.Json, but to be honest you're probably best off using a proper protocol such as gRPC or SignalR.