How to solve this Serialization exception?

Question

I have my code here, I can't understand what is the bug in this. Someone told me the problem is when Deserializing the 'Net' object in the 'Handle Client' method. Because I override it every time a new client comes in. Can you help me solve this? I still can't figure out what should I do.

At first it works for 2-3 messages and then it crashes.

The exception I get is:

Serialization Exception - The input stream is not a valid binary format. The starting contents (in bytes) are: FF-FF-FF-FF-06-44-61-76-69-64-3A-20-66-75-63-6B-20 ... .

Also one time I got the same Serialization Exception only it said - on top object.

The code:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Threading;
using System.Runtime.Serialization.Formatters.Binary;
using System.Net.Sockets;
using System.Net;
using Message;

namespace Chat
{
    public partial class ChatWindow : Form
    {
    uMessage umsg = new uMessage();
    BinaryFormatter bf = new BinaryFormatter();
    NetworkStream Net;
    TcpListener listener;
    TcpClient client;
    List<NetworkStream> Clients = new List<NetworkStream>();

    public ChatWindow(string ip, int port)
    {
        InitializeComponent();
        umsg.IP = ip;
        umsg.Port = port;
    }

    public void OpenNewThread()
    {
        listener = new TcpListener(IPAddress.Parse(umsg.IP), umsg.Port); 
        listener.Start();
        Thread a = new Thread(Listen);
        a.Start();
    }

    public void Listen()
    {
        do
        {
            client = listener.AcceptTcpClient();
            Net = client.GetStream();
            Clients.Add(Net);
            umsg.Name = bf.Deserialize(Net).ToString();
            lstboxCurrentUsers.Invoke(new Action(() =>
            {
                lstboxCurrentUsers.Items.Add($"{umsg.Name} connected at " + DateTime.Now);
                listboxHistory.Items.Add($"{umsg.Name} connected at " + DateTime.Now);
            }));
            LetSubsKnow(Clients);


                Thread b = new Thread(() => HandleClient(Clients));
                b.Start();


        } while (true);
    }

    public void HandleClient(List<NetworkStream> ClientsStream)
    {
        while (true)
        {
            umsg.Message = bf.Deserialize(Net).ToString();

            foreach (var client in ClientsStream)
                {
                   bf.Serialize(client, umsg.Message);
                }
        }
    }

    public void LetSubsKnow(List<NetworkStream> clientsStream)
    {           
        foreach (var client in clientsStream)
        {
            bf.Serialize(client, $"{umsg.Name} Has Connected.");
        }
    }

Answer 1

The Net field keeps getting replaced by the client that has most recently connected, so even though you have one HandleClient thread per client, all these threads read from the most recently obtained NetworkStream.

Similarly the umsg.Name field is overwritten whenever a client connects, and the umsg.Message field whenever a message arrives.

You can fix these issues by supplying the NetworkStream of the individual connection to HandleClient, and creating a local variable for the received message:

public void HandleClient(NetworkStream client)
{
    ...
    string message = bf.Deserialize(client).ToString();
    ...
}

Similarly you will need to pass the name of the client to the LetSubsKnow method, instead of relying on the umsg field that keeps getting updated.

public void LetSubsKnow(string clientName)
{
    ....
}

Furthermore even though you pass the clientsStream around as a parameter, they are all the same reference to the Clients field, and if a client connects while you're sending data, the foreach will throw a "The collection has been modified" exception.

This can be fixed by accessing the Clients field using a lock. I wouldn't put the whole foreach block inside a lock section, but take a snapshot of the currently connected clients instead:

private readonly object clientsLock = new object();
List<NetworkStream> Clients = new List<NetworkStream>();
...
NetworkStream[] currentClients;
lock(clientsLock)
{
    currentClients = Clients.ToArray();
}

foreach (NetworkStream client in currentClients)
{
    // send stuff
}

Note that you're going to need some exception handling around the NetworkStream accessing code.

Last but not least I don't think BinaryFormatter is thread-safe (see this answer). Instead of locking you might be better off creating new ones in the HandleClient and LetSubsKnow methods.