[Bug]: Deadlock using expireAfter

[aguahombre]

Describe the bug 🐞

Create two observables that call ToObservableChangeSet with expireAfter and they may end up in deadlock.
The TaskPoolScheduler thread that handles the item expiration can end up with both scheduled expirations nested on the stack holding locks that deadlock the adding threads.

Step to reproduce

Create a console project and add this code to Program.cs

using DynamicData;
using System.Reactive.Linq;

namespace DynamicDataDeadlock;

internal class Program
{
    static void Main(string[] args)
    {
        Observable.Interval(TimeSpan.FromMilliseconds(250))
            .Select(x => new Item(x % 100))
            .ToObservableChangeSet(x => x.Value, expireAfter: _ => TimeSpan.FromSeconds(1))
            .Subscribe(x => Console.WriteLine($"{string.Join(',', x)}"));
        Observable.Interval(TimeSpan.FromMilliseconds(250))
            .Select(x => new Item2(x % 71))
            .ToObservableChangeSet(x => x.Value, expireAfter: _ => TimeSpan.FromSeconds(1))
            .Subscribe(x => Console.WriteLine($"{string.Join(',', x)}"));

        Console.ReadKey();
    }
}

internal class Item
{
    public Item(long x) => Value = x;

    public long Value { get; }
}

internal class Item2
{
    public Item2(long x) => Value = x;

    public long Value { get; }
}

Run the program in the debugger and it will eventually stop outputting lines. Pause the program and inspect the parallel threads to observe the deadlock.

Removing the console debug will reach deadlock faster.

Reproduction repository

https://github.com/aguahombre/DynamicDataDeadlock

Expected behavior

The two subscriptions run independently and never deadlock.

Screenshots 🖼️

Here is the parallel threads window showing threads 27772 and 12496 in deadlock.

IDE

Visual Studio 2022

Operating system

Windows 10, Linux Bookworm

Version

17.13.6

Device

PC, RPI4B

DynamicData Version

9.2.1

Additional information ℹ️

No response

[JakenVeina]

Oh, thaaaaat is distressing.

On my end, the second subscription seems to be a necessity to trigger this deadlock. Can you confirm? These are two completely independent streams, so that suggests there's some kind of static, ThreadLocal or AsyncLocal state being shared by separate instances of this operator.

[JakenVeina]

Alright, I think I've got a proof-of-concept of the issue. It is indeed a shared-state issue, that shared state being wrapped up inside the shared IScheduler that both operators are implicitly using, via GlobalConfig.DefaultScheduler (which is a TaskPoolScheduler, by default).

The issue stems from the fact that the TaskPoolScheduler has some optimizations built-in, which means it doesn't really do what its name suggests. Instead of always invoking scheduled actions via a Task, pulled from a pool, it will sometimes invoke them on the scheduling thread. I.E. when you make an IScheduler.Schedule() call, the scheduler doesn't just schedule your action and return, it will immediately invoke any actions that were previously-scheduled that are now due, similar to the ImmediateScheduler. This is not a behavior I accounted for when building this operator. It was my assumption that IScheduler.Schedule() would be a "fire-and-forget" operation, so long as you're not using ImmediateScheduler or CurrentThreadScheduler, which require a single-threaded environment anyway.

I proved the concept by inserting a Task.Run() within the .Schedule()d actions that were deadlocking, to force the scheduler to behave the way I thought it would, and the deadlock disappears. I was able to run continuously for about 10 minutes with this adjustment, whereas the deadlock normally appears in 1-2 seconds, after I reduced the interval and expiration timings to 1ms and 4ms, respectively. Just dropping a Task.Run() in here is definitely not a proper solution, so I'll need to do some more thinking, and probably talk to the RX.NET folks.

In the meantime, the other thing I verified is that passing ThreadPoolScheduler.Instance as the scheduler parameter of the operator, instead of leaving it null, is a viable workaround, until we get a proper fix. Same deal as above, I was able to run for several minutes with no deadlock. If you're using this in a severely-hot path, you might get reduced performance with this scheduler, so keep that in mind.

Thanks for the extremely-detailed write-up, BTW.

[aguahombre]

Thanks for looking into this so quickly. I had also found that changing the scheduler to NewThreadScheduler.Default seems to avoid the deadlock. I wonder if there has been a recent breaking change in TaskPoolScheduler, as my code hasn't changed in that area recently and I've only just come across the problem.

[JakenVeina]

I dug into the blame history within TaskPoolScheduler, the "problematic" part (using TaskContinuationOptions.ExecuteSynchronously when creating Tasks for scheduled actions) hasn't changed since its inception. Or, at least, since they migrated all the source to GitHub, they didn't migrate any of the repo history from before that.

Looking at our OWN history, there are two changes jointly responsible for introducing this issue.

Version 8.3.93 included #862, which centralized scheduler usage across the entire library into a configurable GlobalConfig.DefaultScheduler field, for consistency. However, this also changed the scheduler that's actually USED as the default from a DefaultScheduler to a TaskPoolScheduler. DefaultScheduler is actually an entirely different implementation of IScheduler that is built to achieve platform-independence through an IConcurrencyAbstractionLayer implementation that's automatically chosen per-platform. I'd have to spend a lot more time digging to reverse-engineer all the layers of indirection that are involved here, but at a glance, it looks like scheduling is all timer-based instead of Task-based.

Version 8.3.25 included #771, in which the entire .ToObservableChangeSet() operator was re-written from scratch.

Rolling back either of these two changes eliminates the deadlock, so we have a fun little marriage issue. Essentially, #862, which is possibly a defect we should roll back, revealed a probable defect in #771.

So, what you should REALLY do, for now, is use Scheduler.Default rather than ThreadPoolScheduler.Instance

I'll keep digging and figure out what the proper resolution is on our end.

[JakenVeina]

Also going to mention #854 for reference. This is where the decision to change our default scheduler originated.

[aguahombre]

Will this problem also affect the ExpireAfter method?

[JakenVeina]

Sure looks like it.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.