Possible bug in System.Reactive.Windows.Forms/Reactive/Concurrency/ControlScheduler.cs

[forgetaboutit]

Environment: Application running on .NET Framework 4.6 and Rx 2.2.5.0 including the System.Reactive.Windows.Forms shim (code of the current Rx release 3.0.0 and master seems to exhibit the same problem).

I ran into a situation at work where a Form instance has already been closed/disposed of when the ControlScheduler (via ObserveOn(form)) tries to BeginInvoke on it.

ControlScheduler.Schedule<TState>(TState, Func<IScheduler, TState, IDisposable>) explicitly checks in the delegate to BeginInvoke if the control has not been disposed of yet. It doesn't perform this check, however, before scheduling on the control's event loop. I would classify this as a race condition. To my understanding, cancelling subscriptions is best-effort, which leads to the problem I observed (no pun intended).

I fixed the problem by creating a custom scheduler cloned from ControlScheduler and checking the _control field for IsDisposed before calling BeginInvoke.

Current code:

var d = new SingleAssignmentDisposable();

_control.BeginInvoke(new Action(() =>
{
    if (!d.IsDisposed)
        d.Disposable = action(this, state);
}));

My fix:

var d = new SingleAssignmentDisposable();

// Prevent scheduling on disposed controls
if (_control.IsDisposed) return d;

_control.BeginInvoke(new Action(() =>
{
    if (!d.IsDisposed)
        d.Disposable = action(this, state);
}));

I'm not sure if the current behavior is accidential or intentional. It'd be great if an expert on the topic could comment on whether my current fix is a good idea and if it should be fixed in Rx itself or handled by user code.

[GeorgeTsiokos]

Breaks contract but it prevents the exception:

/// <summary>
/// Schedules work until the control is disposed
/// </summary>
public class SafeControlScheduler : ControlScheduler
{
    public SafeControlScheduler([NotNull] Control control) : base(control) { }

    public override IDisposable Schedule<TState>(TState state, DateTimeOffset dueTime, Func<IScheduler, TState, IDisposable> action) => Control.IsDisposed ? Disposable.Empty : base.Schedule(state, dueTime, action);

    public override IDisposable Schedule<TState>(TState state, Func<IScheduler, TState, IDisposable> action) => Control.IsDisposed ? Disposable.Empty : base.Schedule(state, action);

    public override IDisposable Schedule<TState>(TState state, TimeSpan dueTime, Func<IScheduler, TState, IDisposable> action) => Control.IsDisposed ? Disposable.Empty : base.Schedule(state, dueTime, action);
}

[akarnokd]

I'm not familiar with WinForms. What happens when you call BeginInvoke on a disposed Control? Does it throw an exception?

[akarnokd]

https://bytes.com/topic/c-sharp/answers/245294-invokerequired-begininvoke-disposed-control

While you could check on the call before BeginInvoke to see if the form
is disposed, you should also call in the method that is passed to
BeginInvoke as well, when you are on the thread that an Invoke is not
required on. If you are disposed there, then you should not call the rest
of your code, and then exit.

[danielcweber]

@akarnokd Should @forgetaboutit's fix be implemented?

var d = new SingleAssignmentDisposable();

// Prevent scheduling on disposed controls
if (_control.IsDisposed) return d;

_control.BeginInvoke(new Action(() =>
{
    if (!d.IsDisposed)
        d.Disposable = action(this, state);
}));

[akarnokd]

To be on the safe/eager side, we could implement this:

var d = new SingleAssignmentDisposable();

// Prevent scheduling on disposed controls
if (_control.IsDisposed) return d;

_control.BeginInvoke(new Action(() =>
{
    if (!_control.IsDisposed && !d.IsDisposed)
        d.Disposable = action(this, state);
}));

[danielcweber]

Disposable.Empty should suffice if the control is disposed?

[akarnokd]

You mean?

// Prevent scheduling on disposed controls
if (_control.IsDisposed) return Disposable.Empty;

var d = new SingleAssignmentDisposable();

_control.BeginInvoke(new Action(() =>
{
    if (!_control.IsDisposed && !d.IsDisposed)
        d.Disposable = action(this, state);
}));

return d;

[danielcweber]

yes

[glopesdev]

@akarnokd @danielcweber I am sorry for the late comment, but I think silently ignoring these scheduling exceptions is the wrong approach, as they can lead to situations where issues are much harder to debug.

The original behavior of BeginInvoke is to throw when a call is made on a disposed control. This means that the caller did not bother to keep track of the state of the control itself before scheduling, so the error is there to raise awareness to the developer. I have faced this issue a number of times before, and while it is indeed annoying, it is easily solvable by cancelling the subscription on the control HandleDestroyed event.

Furthermore, in multi-threading race conditions, testing for IsDisposed actually tells you very little, since you may be lucky enough to pass the test, only to find the exception being raised in the actual scheduling line. If BeginInvoke is being used, this almost always means you are in a different thread (or else why would you call it?). This means that callers of this method may still face unwanted exceptions, except that they will be much harder to find.

I believe this has introduced a regression which will be hard to test since there is a race condition involved, and I strongly believe the behavior should be reverted back to its original logic.