File Transfer over TCP

Question

I'm working on an Asynchronous TCP Client/Server using the old BeginXXX and EndXXXSocket API.

The goal of this hobby project is to have a working multi user chat with file sharing capabilities.

Here's my code to Send the file

public class Client
{
    public readonly AutoResetEvent SendSync = new AutoResetEvent(true);

    public Action<Client, byte[]> OnReceive;
    public Action<Client, double> OnSpeedMeasurement;
    public Action<Client> OnConnect;
    public Action<Client> OnDisconnect;

    public readonly Socket Socket;
    public IPEndPoint ServerEndpoint;

    public volatile bool SocketConnected;
    public readonly object Reference;
    public readonly byte[] Buffer;
    public int BytesInBuffer;

    public Client(object arg)
    {
        if (arg is Socket)
            Socket = (Socket)arg;
        else
        {
            Socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
            Reference = arg;
        }

        Buffer = new byte[ushort.MaxValue];
    }

    public Task ConnectAsync(string ip, ushort port)
    {
        ServerEndpoint = new IPEndPoint(IPAddress.Parse(ip), port);
        return Task.Factory.FromAsync(Socket.BeginConnect(ServerEndpoint, null, Reference), Connected);
    }

    private void Connected(IAsyncResult ar)
    {
        try
        {
            Socket.EndConnect(ar);
            SocketConnected = true;
            OnConnect?.Invoke(this);
            Receive();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            Disconnect();
        }
    }

    public void Receive()
    {
        try
        {
            if (!SocketConnected)
                throw new SocketException((int)SocketError.NotConnected);

            Socket.BeginReceive(Buffer, 0, Buffer.Length, SocketFlags.None, out SocketError error, Received, Reference);

            if (error != SocketError.Success)
                Disconnect();
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
            Disconnect();
        }
    }

    private void Received(IAsyncResult ar)
    {
        try
        {
            var size = Socket.EndReceive(ar, out SocketError error);

            if (size == 0 || error != SocketError.Success)
            {
                Disconnect();
                return;
            }

            var packet = new byte[size];
            System.Buffer.BlockCopy(Buffer, 0, packet, 0, size);

            OnReceive?.Invoke(this, packet);

            Receive();
        }
        catch (Exception e)
        {
            Disconnect();
            Console.WriteLine(e);
        }
    }

    public void Send(byte[] packet)
    {
        SendSync.WaitOne();
        Socket.BeginSend(packet, 0, packet.Length, SocketFlags.None, EndSend, null);
    }

    private void EndSend(IAsyncResult ar)
    {
        Socket.EndSend(ar);
        SendSync.Set();
    }

    public void Disconnect()
    {
        if (Socket.Connected)
        {
            SocketConnected = false;
            Socket.Close();
            OnDisconnect?.Invoke(this);
        }
    }
    /// <summary>
    /// Sends a File to the Server
    /// </summary>
    /// <param name="path">Path to the file to send</param>
    public void SendFile(string path)
    {
        using (var fileStream = File.Open(path, FileMode.Open, FileAccess.Read))
        {
            var fileName = Path.GetFileName(path);
            var fileSize = fileStream.Length;
            var lastUpdateTime = DateTime.Now;
            var iterations = 0;

            while (fileStream.Position != fileStream.Length)
            {
                using (var memoryStream = new MemoryStream())
                {
                    using (var writer = new BinaryWriter(memoryStream))
                    {
                        // Make the chunks as big as possible, let the TCP Stack do the rest
                        // Preserve 128 + 24 bytes for Header information (128 bytes for filename, 24 bytes for header info).
                        var chunk = new byte[ushort.MaxValue - (128 + 24)]; 
                        // [Offset 0] Advance by 2 bytes, first two bytes are packet size, calculated at the end.
                        writer.Seek(2, SeekOrigin.Current);
                        // [Offset 2] Packet Id, 3000 = FileTransfer
                        writer.Write((ushort)3000);
                        // [Offset 4]
                        writer.Write(Path.GetFileName(fileName));
                        // [Offset 6] FileName to be Created (1) / Appended (0) (6-8 = Length Prefix, 9-N = string)
                        writer.Write(fileStream.Position == 0);

                        //Read the payload (file contents chunk) into a buffer
                        var readBytes = fileStream.Read(chunk, 0, chunk.Length);
                        //Resize the buffer to the correct size
                        Array.Resize(ref chunk, readBytes);

                        // [Offset 6 + FileName Length] Write the total file size
                        writer.Write(fileSize);
                        // [Offset 10 + FileName Length] Write size contained in this packet
                        writer.Write((ushort)chunk.Length);
                        // [Offset 12 + FileName Length] Write payload buffer
                        writer.Write(chunk, 0, chunk.Length);

                        var pos = writer.BaseStream.Position;
                        writer.Seek(0, SeekOrigin.Begin);
                        // [Offset 0] Write the full packet size to the first two bytes
                        writer.Write((ushort)pos);

                        // Get the complete packet out of the stream
                        var buffer = memoryStream.ToArray();
                        // Send it
                        Send(buffer);

                        // Calculate Transfer Speed
                        if (lastUpdateTime.AddSeconds(1) < DateTime.Now)
                        {
                            iterations++;
                            var interval = DateTime.Now - lastUpdateTime;

                            var downloadSpeed = (int)Math.Floor(fileStream.Position / interval.TotalSeconds) / iterations;
                            OnSpeedMeasurement?.Invoke(this, downloadSpeed);
                            lastUpdateTime = DateTime.Now;
                        }
                    }
                }
            }
        }
    }
    /// <summary>
    /// Receives a File
    /// </summary>
    /// <param name="packet">packet containing file data</param>
    public void ReceiveFile(byte[] packet)
    {
        using (var reader = new BinaryReader(new MemoryStream(packet)))
        {
            var length = reader.ReadUInt16();
            var id = reader.ReadUInt16();
            var name = reader.ReadString();
            var create = reader.ReadBoolean();
            var dataSize = reader.ReadInt64();
            var dataChunkSize = reader.ReadUInt16();

            var data = new byte[dataChunkSize];
            var read = reader.Read(data, 0, dataChunkSize);

            using (var filestream = new FileStream(@"C:\Test\" + name, create ? FileMode.Create : FileMode.Append))
            {
                filestream.Write(data, 0, data.Length);
            }
        }
    }

That's how the Server processes incoming packets:

public class ServerSocket
{
    public static Action<Client, byte[]> OnPacket;
    public uint LastUniqueId;
    public Socket Socket;

    public void Start(ushort port)
    {
        Socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
        Socket.Bind(new IPEndPoint(IPAddress.Any, port));
        Socket.Listen(1);
        Socket.BeginAccept(Accepted, null);
    }

    private void Accepted(IAsyncResult ar)
    {
        var clientSocket = Socket.EndAccept(ar);
        Socket.BeginAccept(Accepted, null);

        var c = new Client(clientSocket);
        AlumniCollections.ConnectedClients.TryAdd(LastUniqueId++, c);
        Receive(c);
    }

    private void Receive(Client c) => c.Socket.BeginReceive(c.Buffer, 0, c.Buffer.Length, SocketFlags.None, Received, c);
    private void Received(IAsyncResult ar)
    {
        try
        {
            var client = (Client) ar.AsyncState;
            var size = client.Socket.EndReceive(ar, out SocketError error);
            client.BytesInBuffer += size;

            if (size == 0 || error != SocketError.Success)
                client.Disconnect();

            ConstructPacket(client);
        }
        catch (Exception e)
        {
            Console.WriteLine(e);
        }
    }

    private void ConstructPacket(Client client)
    {
        if (client.BytesInBuffer > 2)
        {
            var packetSize = BitConverter.ToUInt16(client.Buffer, 0);

            if (client.BytesInBuffer < packetSize)
                client.Socket.BeginReceive(client.Buffer, client.BytesInBuffer, packetSize - client.BytesInBuffer, SocketFlags.None, Received, client);
            if (client.BytesInBuffer == packetSize)
            {
                client.BytesInBuffer -= packetSize;

                var packet = new byte[packetSize];
                Buffer.BlockCopy(client.Buffer, 0, packet, 0, packetSize);
                OnPacket?.Invoke(client, packet);
                Receive(client);
            }
            if (client.BytesInBuffer > packetSize)
            {
                client.BytesInBuffer -= packetSize;
                var packet = client.Buffer.Take(packetSize).ToArray();
                OnPacket?.Invoke(client, packet);
                var data = client.Buffer.Skip(packetSize).Take(client.Buffer.Length - packetSize).ToArray();

                Array.Clear(client.Buffer, 0, client.Buffer.Length);

                for (var i = 0; i < data.Length; i++)
                    client.Buffer[i] = data[i];

                ConstructPacket(client);
            }
        }
        else
        {
            client.Socket.BeginReceive(client.Buffer, client.BytesInBuffer, client.Buffer.Length - client.BytesInBuffer, SocketFlags.None, Received, client);
        }
    }
}

Might want to point out that the transfers are really slow for some reason.

Answer 1

Client Side

Glad to see inline documentation on SendFile and ReceiveFile. Code is mostly easy to read, I'd have no style complaints.

I haven't much complaint about these methods, so this is mostly performance considerations and commentary on the protocol. Other than buffer usage, everything else is pretty 'petty' and is more food-for-thought than a serious suggestion to increase the complexity of the code.

SendFile(string path) and protocol

I'm assuming you've paid particular attention to the possibility of concurrent calls to Send(byte[]) (I'm assuming that is the signature). This looks sounds, from the additional code provided by the OP

I note that there is no way to indicate the end of a file-transfer. This limits some of your server-side options, and makes it impossible to trust any file transfer from a connection that wasn't cleanly terminated.

Buffer reuse

You should really be re-using your buffers. The chunk buffer and MemoryStream (and Writer) can all be reused. This is the only obvious performance concern I can see in this method.

Moving the using clauses out-side of the while, and simply clear the Memory stream at the end of each loop would suffice. I don't know enough about the buffering to comment on the cost of the .ToArray() call, but you may well want to 'tune' your buffer parameters.

Filename

Taking a shot in the dark... you are subtracting 128 bytes for the Filename, but a 128 byte string would take up 130 bytes, I believe (2 bytes (ushort) for the length of the string (msdn)). I'm assuming you forgot the length rather than the length being (implicitly) capped at 126, but I note that GetFileName()` provides no guarantees. You may want to reconsider this assumption, which I suspect is at best OS specific. If your code is supplied with a longer filename, then the packet length computation will go wrong.

Also this comment looks wrong:

// [Offset 6] FileName to be Created (1) ...

I think it is meant to read [Offset 4 + FileName Length]

If you make your chunk buffer slightly larger, you can use an 'left over' space from a short file-name for more chunk data. This will requiring measuring the filename, which can be done with a UnicodeEncoding and the GetBytes or GetByteCount method (encoding is cheap, but you could save a little time by only encoding once, and perhaps even not clearing the memory stream buffer, and just leaving that memory where it is).

Array.Resize(ref chunk, readBytes);

This resizing is needless: all the methods concerned allow you to provide an offset and length, so there is no need to allocate a new array and make an extra copy (eliminating this enables chunk reuse).

//Read the payload (file contents chunk) into a buffer
var readBytes = fileStream.Read(chunk, 0, chunk.Length);

// [Offset 6 + FileName Length] Write the total file size
writer.Write(fileSize);
// [Offset 10 + FileName Length] Write size contained in this packet
writer.Write((ushort)readBytes);
// [Offset 12 + FileName Length] Write payload buffer
writer.Write(chunk, 0, readBytes);

You could also get at the byte[] that the MemoryStream is using (via [GetBuffer()], and read straight into it. This would reasonably require providing the array in the first place of a known maximum size, and would require manually adjusting the logical length of the stream. This would be ugly and error prone, so I wouldn't suggest it unless the cost of the extra copy is demonstrably a significant concern in the application.

32bit 'data size'

writer.Write(chunk, 0, chunk.Length);

This will necessarily fit into 2 bytes (has to, else the packet would have to be larger), so you can squeeze an extra 2 bytes into each chunk.

Transfer Speed Stuff

This code is not so nice. It isn't clear what it is meant to do (algorithmically), I have a feeling that it isn't doing what you really wanted, and it just generally looks untidy.

ReceiveFile(byte[] packet)

Depending on what the calling code looks like, this could take a Stream instead of a byte[]. Since you have to create a MemoryStream anyway, perhaps accepting a Stream as a parameter would allow greater efficiency when a Stream can be passed (you can provide an overload that takes a byte[] and passes a MemoryStream to the other overload if suitable).

Repeatedly opening the file you are trying to write to may incur significant overheads (forces the file to be flushed when you dispose it, etc.). If you were to provide an indicator that a file-transfer had completed, you could keep 'pending' file streams open, and only close them when the file transfer completes, is interrupted (which is currently indistinguishable from completion), or disposed owing to some bizarre resource constraint (not a concern for a 'hobby' project, unless the whole point is to learn about caching!).

Server side

This code is less pretty, but readable enough. Even though these method are not exposed, they could do with inline doc for the maintainer's benefit. It is less than clear that ConstructPacket calls client.Socket.BeginReceive (ConstructPacket doesn't strike me as a great name). It is also not clear how these methods are to be used: it looks like a single call to Receive will set off a loop of activity across all manner of threads, and I don't really know the performance implications of this. The usage of client.Buffer is unclear, and if any of these 3 methods were to be separated, working out what it is doing would be painful.

Petty style stuff: you have a couple of ifs and a for in there without a pair of curly braces {}, which is a good way to annoy people, and personally I'd put a line-break after every if just to space them out a bit (otherwise they feel like they should be elses, and it requires extra effort to scan down through the conditions).

I would be inclined to put all of these methods in their own class (is this a sub-class of Client already?), or indeed inside the Client itself (should Client.Socket really be public? I don't know, but it bothers me that such an intricate entity appears to be exposed (same goes for Client.Buffer).

try...catch

This try...catch looks like it is there for 'casual' debugging because the client that died didn't really matter I just want to know when a thread crashes, but it isn't clear what exception this is hoping to catch, or why it is trying to handle it. It looks like it could do with disconnecting the client, since it effectively cuts them off by ending the Receive -> Received -> ConstructPacket cycle. Certainly give this some serious thought if you haven't already. (Should the other client's not be informed when a client is lost? At the very least, there must be some server state to clean up)

client.Buffer.Take/Skip

LINQ is sllloooowww!!! You'd do much better to create the packet byte[] explicitly and use block copies from the buffer (rather than IEnumerable.ToArray() and the copy-loop) to the packet array and to itself.

The byte[] for the packet could be cached and used for each call of ConstructPacket for a given client. Unless it somehow ends up in Gen2/LOH it shouldn't be a huge performance drag, however.