Records and concurrency

[jeremysheeley]

I'm experimenting with records, and I think that concurrency will cause some real confusion and bugs for people, when they start using records to hold their UI state. Here's some code:

public record TestThing
{
    public string First { get; init;}
    public string Second { get; init; }
}

public async Task<string> LongRun()
{
    await Task.Delay(1000);
    return "This took a while.";
}
async void Main()
{	
    var recLock = "lock";
    //These are both instances that could be described as "You can no 
    //longer update independent properties from different threads." 
    //This is just two ways to get the same issue. The first example 
    //is the worst. Don't await inside a with block. Your record will 
    //probably be stale when you get back. I would vote for a compiler 
    //warning or error.
    var rec = new  TestThing { First = "never updated", Second = "never updated" };
    bool awaitOutside = false;
    var firstTask = Task.Run(async () => {
        if (awaitOutside) //No data will be lost.
        { 
            var val = await LongRun();
            //Can lock here, if you want
            rec = rec with { First  = val };
        }
        else //data will be lost
        {
            //lock(recLock) //Can't lock here, since you can't await inside a lock
            {
                rec = rec with { First  = await LongRun() };
            }
        }
    });
    await Task.Delay(500); //Just to give the firstTask time to start.
    rec = rec with { Second = "New value" };
    await firstTask;
    //The record will have Second set to "never updated"
    Console.Out.WriteLine(rec);
    
    
    //This is the same thing again, but faster, and without an await.
    rec = new  TestThing { First = "a", Second = "b" };
    var lockDuringUpdate = false; //Only works reliably if you lock during update.
    firstTask = Task.Run(() => {
        for (int i = 1; i < 1000; i++)
        {
            if (lockDuringUpdate)
                lock(recLock)
                {
                    rec = rec with { First = rec.First + "a" };
                }
            else
                rec = rec with { First = rec.First + "a" };
        }
    });
    
    var secondTask = Task.Run(() => {
        for (int i = 1; i < 1000; i++)
        {
            if (lockDuringUpdate)
                lock(recLock)
                {
                    rec = rec with { Second = rec.Second + "b" };
                }
            else
                rec = rec with { Second = rec.Second + "b" };
        }
    });
    await firstTask;
    await secondTask;
    Console.Out.WriteLine("rec has first length " + rec.First.Length + 
                          " and second length " + rec.Second.Length);
    Console.Out.WriteLine(rec);
}

[YairHalberstadt]

I'm really not sure what you're saying will happen here, and what is expected?

[canton7]

The thing is, you're not just mutating a property. You're re-assigning rec. That's always going to be racey, regardless of whether records are involved or not.

I don't think records are at fault here specifically: any immutable object is going to have the same issue. Heck, multithreaded lockless manipulation of mutable objects is normally going to have the odd race condition hiding away.

Also, no one should be manipulating UI state from a non-UI thread. Ever, regardless of records. Always marshal back to the UI thread before doing anything like that. WPF will (un)helpfully marshal INPC events to the UI thread for you most of the time (but you're still open to race conditions if you try), but won't do the same for many other things, including INCC. Most other UI frameworks will throw, or just silently go bad.

(As an aside, you can use a SemaphoreSlim in an async method, as it doesn't have thread affinity. As a bonus, it has a WaitAsync method)

[jeremysheeley]

Thanks for the pointers, canton7, there are definitely things for me to look at there.

Mostly, my feeling is that these situations are traps waiting for the unsuspecting. In particular, I don't see any value in allowing await inside a with block. The record reference is set at the beginning of the with, and the UI thread is free to change the record reference while the await is happening. That means that any changes that happen during the await are guaranteed to be lost.

Consider a UI with these two events:

        private void Faster_Click(object sender, RoutedEventArgs e)
        {
            rec = rec with { Second = "New value" };
        }

        private async void Slower_Click(object sender, RoutedEventArgs e)
        {
            rec = rec with { First = await LongRun() };
        }

If you click the Slower button, and then click the Faster button, the end result is that the First property will be updated, but the faster change to the Second property will be lost. This is only because the await is inside the with block. If the compiler forced or encouraged me to move the await up, then no changes would be lost.

[canton7]

Yeah, I can see there being an argument for rec being evaluated after all of the arguments to the wither are evaluated. I can't immediately see a reason why rec should be evaluated first, but it's very likely that I'm missing something though.

Not necessarily as a thread safety argument, but more in case one of the wither arguments re-assigns the record as a side effect.

[CyrusNajmabadi]

Tagging @jcouv what are our rolls here for evaluation of the 'with' receiver and the assignment to the lvalue? I would expect to this to be similar to be how we do object initializers. In this case I could see though it's evaluating all the initializers first, then grabbing the receiver and doing the clone. Or I could see us grab the receiver first.

This is possibly worth a quick ldm email

[CyrusNajmabadi]

, I don't see any value in allowing await inside a with block

I see a ton of value in that. I would find it exceptionally unfortunate if you could not do that.

There is an important discussion to be had though about evaluation order here. And, in general, you will always need to be careful if you're partially evaluating and assigning in async contexts

[jeremysheeley]

, I don't see any value in allowing await inside a with block

I'm sorry for speaking so absolutely. I certainly can't claim any authority, especially among this crowd. To further clarify my point:

var val = await LongRun();
rec = rec with { First = val };

is pretty clear and predictable.

rec = rec with { First = await LongRun() };

has some very subtle things going on, and at the moment is somewhat dangerous.

I hadn't even considered what happens if the called method changes the record reference, like @canton7 mentioned. That would move this conversation away from concurrency. since await isn't necessary.

        private string UpdateRecRef()
        {
            rec = rec with { Second = "UpdateRecRef called" };
            return "Returned From UpdateRecRef";
        }
        private void Slower_Click(object sender, RoutedEventArgs e)
        {
            rec = rec with { First = UpdateRecRef() };
            //The record with a change to rec.Second is lost here.
        }

[CyrusNajmabadi]

I don't really understand what records have to do with this though. As I mentioned above:

in general, you will always need to be careful if you're partially evaluating and assigning in async contexts

You are referencing a value in a certain location, hit also assigning over that location. This is always something you need to be careful around.

[333fred]

I would expect to this to be similar to be how we do object initializers.

I would expect the same, it should be equivalent to new Rec() { Prop = 1 }, which will run the constructor first. And indeed, that is our behavior: https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxAgrgOyQExAagB8ABAJgEYBYAKBoDcIEACAYyYF4nM4B3JgYQAUASiYBvJgEkOTAOJwYkzDBFMAvgG56jJjhnc+AERFbqAfRl6eASxgALcVJnzFy1Zpo1ryuQqUrhGjEaJlCmYnIATkEAIhd/ERjhUzDwgHYmclM1T2piAGYmBDgWAHsEPUMgkLCCpmNRMRzqVLrjQ11AlrDg7tTQiOiYjrKABwBPVlLMWAwWGHKklLDm1JqBwu8YJwkAcwUNJigD9RpmurIBcXXwwqEu1N7+2qjY/imZmDmFhCWb1bCNzqWx2TH2MEOxwhp2oaiAA

[YairHalberstadt]

I also wouldn't expect this to be different from
rec = rec.With(Prop: await M()) which evaluates rec first.

[canton7]

On reflection, I would be very surprised if this code created Rec() { Member = "first" } and not Rec() { Member = "second" }, so I retract what I said above:

record Rec
{
    public string Member { get; set; } 
} 

class C
{
    private string s = "first";

    private Rec GetRec() 
    {
        s = "second";
        return new Rec();
    } 

    public void M() 
    {
        rec = GetRec() with { Member = a };
    } 
} 

[jeremysheeley]

I worked up a slightly more real-world example of the await-less version of this issue. Nothing that the compiler is doing now is unexpected. It is all in-line with the way that everything else in C# behaves. It's just that the end result of all of this expected behavior is disappointing. I view the await-less version to be "irritating, but I should have programmed it better". The await version is much more serious, since I have no control over what state changes will be applied (and subsequently lost) while the await is going.

using System;

var stateRecord = new StateRecord() { LatestTweet = 99, InternetAvailable = true };
Console.WriteLine(stateRecord);
stateRecord = stateRecord with { LatestTweet = GetLatestTweet() };
Console.WriteLine(stateRecord);

int GetLatestTweet()
{
    //Oh no, the internet is off!
    stateRecord = stateRecord with { InternetAvailable = false };
    Console.WriteLine(stateRecord);
    return -1;
}

public record StateRecord
{
    public int LatestTweet { get; set; }
    public bool InternetAvailable { get; set; }
}

Results in

StateRecord { LatestTweet = 99, InternetAvailable = True }
StateRecord { LatestTweet = 99, InternetAvailable = False }
StateRecord { LatestTweet = -1, InternetAvailable = True }

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxAgrgOyQExAagB8ABAJgEYBYAKBoDcIEACWCeAJTgGMB7BHJgF4mmOAHcmAZRhs4nXvwAUASiYBvJgBlZsACpi4cGEKYBOU0iYBJTPASiYAQQYBLADYRgbuCZgYfAL4A3DTE5KaKrBzcfDjKIdRRcjH8JknysUxiLjAAFupaOjD6hsbCAOJG2vB6BkYqTMGh4ZEy0QpxCTQutkyVMNVwtaUqNGo0TJNMAPTTAPL5mDyWeT49dg5MLlBMPABmewCEE1PpKQLCZx1ZOfkaNhtGzhDunt4mexBuUIEJU0xhCJXWLxE6TYgAdiYAFpyAkAjRQgBmJgIc5SNrJDpjMEAlHrQo1Yp1YwaADmRiCLEpjVxxBRwB4PDc1lsiAcz1eXh85JpPxgVIR1ACQA

[HaloFour]

This kind of problem is always possible when mutating shared state in multiple locations, regardless of threading. GetLatestTweet should be returning a new instance of StateRecord, not modifying it in one place and being used to modify it in another. This same behavior would occur if you were using nearly any "immutable" data type and has been the case since C# 1.0.

[jeremysheeley]

I've been experimenting with F# and fabulous to see if I can get some of the same behavior there. The framework blocks the dumb thing I'm doing in two ways.

1. Hiding the model reference. That means no updating the model as a side-effect. You have to return the new model from the update function.
2. The update function returns a model, so you can't have an async block in it. Async happens inside commands.

In addition, I can't find a way to await inside a with in F#. It's entirely possible there is one, and I'm missing it. I haven't got a lot of experience with F#.

[CyrusNajmabadi]

I think halo put it very well:

GetLatestTweet should be returning a new instance of StateRecord, not modifying it in one place and being used to modify it in another. This same behavior would occur if you were using nearly any "immutable" data type and has been the case since C# 1.0.

This is not an issue with records. This is an issue with how you are managing state and mixing that with async/await. This is a recipe for badness no matter what. You could have completely Immutable data and this would be a problem.

Heck, afaict, it's just a classical race condition. Similar if you did i++ in multi threaded code without ensuring safety.

[jeremysheeley]

Wrapping this conversation up...

Don't do what I did.

• There is not a bug here. Everything is working as designed, but there is a pitfall that devs need to be wary of.
• If you use a record for your state, you must be 100% certain that every state change is started and completed before the next state change can begin. UI frameworks will hopefully help enforce that.
• Only use await in a with block if you are certain that your record reference can't be changed elsewhere during the await. (Personally, I will round this to "Never use await in a with block")

I'm still interested to know if await inside a with block is something that is possible in F#. It seems like a bad idea to release the UI thread to make other state changes while you are in the middle of one.

Thank you all for the insight and clarification.

[jnm2]

The rule of thumb I use for UI is to recheck absolutely everything after every await. I started using a homespun MVU pattern a few years ago in such a way that await no longer appears in UI code. Instead, actions (descriptive structs) are dispatched between the UI component and a data access component. I've been very happy with this. It's much harder to miss concurrency issues.