C# static function is sending wrong arguments to function

Question

My code is passing the wrong argument to a function for some reason.

I have a static class say class A having this function AddMaster :

public static void AddMaster(string ipAddress, int port, List<RegisterMap> registers)
{

    // THIS LINE PRINTS THE ACTUAL VALUES SENT FROM THE CALLER FUNCTION
    System.IO.File.AppendAllText("datalog_MB.txt", ipAddress + "   " + registers[0].FriendlyName + "\n");
    
    new Thread(() =>
    {
        _tasks.Add(Task.Factory.StartNew(() =>
        {
            Monitor.Enter(_masters);
            _masters.Add(new Master().Connect(ipAddress, port).SetRegisters(registers));

            _masters.Last().OnEvent += MasterEvent;

            Debug.WriteLine(_masters.Count + " TCP masters connected");
            Monitor.Exit(_masters);
        }));

    }).Start();

}

I have another non-static class Master having the function SetRegisters:

public Master SetRegisters(List<RegisterMap> registerList)
{
    // HERE THE FriendlyName ALWAYS PRINTS THE VALUE OF THE LAST FUNCTION CALL
    System.IO.File.AppendAllText("datalog_MB_1.txt", _hostname + "   " + registerList[0].FriendlyName + "\n");
    _registersToRead = registerList;
    return this;
}

The function AddMaster() is called in a loop.

The first code logs the following which is correct:

# datalog_MB.txt
192.168.0.12   192.168.0.12:Value 1
192.168.0.11   192.168.0.11:Value 1

However the second code block prints the following ( See the second value has changed ):

# datalog_MB_1.txt
192.168.0.12   192.168.0.11:Value 1
192.168.0.11   192.168.0.11:Value 1

Edit #1

foreach (var equipment in MSSQL.GetEquipments(true)) 
{
    registers.Clear();
    
    System.IO.File.AppendAllText("dataeq.txt", equipment.IPAddress + "    " + equipment.Name + "     " + equipment.ID +  "\n");
    
    try
    {
        registers.Add(
            new RegisterMap
            {
                FriendlyName = equipment.IPAddress + ":Value 1",
                Register = 2001,
                Type = RegisterType.HoldingRegister,
                StationID = 1
            });
        
        registers.Add(
            new RegisterMap
            {
                FriendlyName = equipment.IPAddress + ":Value 2",
                Register = 2002,
                Type = RegisterType.HoldingRegister,
                StationID = 1
            });

        A.AddMaster(equipment.IPAddress, 502, registers);

        var json = new JavaScriptSerializer().Serialize(registers);
        
        System.IO.File.AppendAllText("data_reg.txt", json + "\n\n");
    }
    catch(Exception err)
    {
        System.Windows.MessageBox.Show(err.Message);
    }
}

Edit #2*

Fiddle: https://dotnetfiddle.net/h3yn7p

Any idea what might be going wrong?

"My code is passing the wrong argument to a function for some reason." I think that's highly unlikely. I think it's much more likely that you've got multiple threads modifying the same list, and possibly modifying objects within that list, in a way that isn't thread-safe. (Aside from anything else, your use of `Monitor.Enter` and `Monitor.Exit` is highly suspect - if anything throws an exception between them, you'll never call `Monitor.Exit`.) We can't see the surrounding code which is relevant - if you can provide a [mcve] it will be much easier to help you diagnose the issue. — Jon Skeet, Jul 09 '22 at 07:15
@JonSkeet `Monitor` i've used cuz of the issue i was facing here https://stackoverflow.com/q/72679914/5437621 — mrid, Jul 09 '22 at 07:32
So use a `lock` statement instead - that's significantly safer than what you've got now. Although it's still not clear whether `RegisterMap` is mutable or not, and how you're protecting against *that* being mutated. But fundamentally there's still the same problem: we can only see part of the code, so we can't reproduce the problem. — Jon Skeet, Jul 09 '22 at 07:48
Can you show how you call `AddMaster`? You are calling it twice, right? — Klaus Gütter, Jul 09 '22 at 07:48
@KlausGütter I've updated my question to add that code as well. I'm also trying to create a fiddle if that helps — mrid, Jul 09 '22 at 07:53
So you modify the registers list in the loop while the started task using it still runs. You could try to create a new list each time instead. — Klaus Gütter, Jul 09 '22 at 07:57
@JonSkeet I've updated the question with a fiddle : https://dotnetfiddle.net/h3yn7p — mrid, Jul 09 '22 at 08:02
@KlausGütter I tried that in my fiddle and it did work indeed. But I still don't get it why wasn't working — mrid, Jul 09 '22 at 08:08
Because you are re-using and modifying the list while it is still in use by the previously created tasks. — Klaus Gütter, Jul 09 '22 at 08:10
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/246291/discussion-between-mrid-and-klaus-gutter). — mrid, Jul 09 '22 at 08:11

score 0 · Answer 1 · edited Jun 18 '23 at 19:41

0

You should not Clear the registers - recreate it:

List<RegisterMap> registers = null;
foreach(var equipment in equipments) 
{
    registers = new List<RegisterMap>(); 
    ....
}

Otherwise you are processing the same instance of List<RegisterMap> in parallel in multiple threads while manipulating it (i.e. thread which has the foreach(var equipment in equipments) loop will run and create all the threads and start them), which can lead to a lot of different concurrency issues.

Also some notes:

Consider using concurrent collections, for example ConcurrentBag<T> instead of Monitor + List (because in current code it makes Thread/Task handling pointless)
Based on the provided code there is no need to create Thread - tasks should be enough. In modern .NET there is rarely a case when you need to create Threads manually, most of the time it is hugely discouraged (read more).

edited Jun 18 '23 at 19:41

halfer

19,824
17
99
186

answered Jul 09 '22 at 08:12

Guru Stron

102,774
10
95
132

Can you please tell me the reason? Shouldn't it run synchronously? – mrid Jul 09 '22 at 08:14
@mrid why it should run synchronously? You are not blocking the loop which creates tasks/threads and it will clear and refill list on every iteration. – Guru Stron Jul 09 '22 at 08:15
AddMaster will create a task (TaskFactory.StartNew) which then runs asynchronously (and it creates this task even unneccessarily from a separate thread, so "double-asynchrounously"). – Klaus Gütter Jul 09 '22 at 08:17
Aah I think my understanding of the loop may be wrong. I thought the code block would run one iteration after the other and each time the first line would reset the registers – mrid Jul 09 '22 at 08:20
1

@mrid if you want it blocked, why do you need `Thread`/`Task` at all? – Guru Stron Jul 09 '22 at 08:25
@GuruStron so the classes `A` and the one with the function `AddMaster` are working on networking so need to be async. I was just thinking of initializing these classes one by one. Thereafter they can run in parallel. – mrid Jul 09 '22 at 08:41
@GuruStron also I have one small doubt. If everytime the wrong set of registers are being sent why does the `AddMaster` print the correct registers but `SetRegisters` prints wrong in my fiddle https://dotnetfiddle.net/h3yn7p – mrid Jul 09 '22 at 08:59
@mrid because `AddMaster` prints before the collection is changed. And `SetRegisters` - after. – Guru Stron Jul 09 '22 at 09:22

C# static function is sending wrong arguments to function

1 Answers1