dotnet / dotnet/reactive

Possible Deadlock in BehaviorSubject.Subscribe

Open
#2,080 2 comments 0 reactions 0 assignees View on GitHub
[area] Rx
Dominant language
C#
Stars
7.2k
Forks
798
PR merge metrics
No merged PRs in 30d

Description

Hello and thank you for using dotnet/reactive. Please select a category and detail your issue by answering the questions there:

#### Bug

We recently had a deadlock in our application, when while the Subscribe was active, the UnSubscribe (via the Dispose) was called on a differen thread and this caused a deadlock. This happened in our case as the subscription was cancelled before the Subscribe was even called (due to a quickly opening and closing of a screen).

We found the issue to be in the following file: https://github.com/dotnet/reactive/blob/main/Rx.NET/Source/src/System.Reactive/Subjects/BehaviorSubject.cs

I suggest to change the following code from
```cs
public override IDisposable Subscribe(IObserver observer)
{
if (observer == null)
{
throw new ArgumentNullException(nameof(observer));
}

Exception? ex;

lock (_gate)
{
CheckDisposed();

if (!_isStopped)
{
_observers = _observers.Add(observer);
observer.OnNext(_value);
return new Subscription(this, observer);
}

ex = _exception;
}

if (ex != null)
{
observer.OnError(ex);
}
else
{
observer.OnCompleted();
}

return Disposable.Empty;
}
```

To:
```cs
public override IDisposable Subscribe(IObserver observer)
{
if (observer == null)
{
throw new ArgumentNullException(nameof(observer));
}

Exception? ex;

lock (this._gate)
{
this.CheckDisposed();

this._observers = this._observers.Add(observer);
ex = this._exception;
}

// Note: This first IF statement with the observer.OnNext was originally inside the lock
// This could cause a deadlock in certain circumstances. As part of the fix we have made a copy of the
// BehaviorSubject from Reactive.Net and moved this outside the lock to prevent a deadlock
// when a subject is being disposed while being inside the Subscribe.
// See also: https://dev.azure.com/bronkhorst/IT/_wiki/wikis/Software-development.wiki/627/043-deadlock-terug-navigeren-Android
if (!this._isStopped)
{
observer.OnNext(this._value);
return new Subscription(this, observer);
}
else if (ex != null)
{
observer.OnError(ex);
}
else
{
observer.OnCompleted();
}

return Disposable.Empty;
}
```

The main change is that the IObserver.OnNext call is outside the lock. This also matches what happens when BehaviourSubject.OnNext from the is called, where the IObserver.OnNext is also outside the lock. This will prevent future deadlock scenarios.

> Which library version?
6.0.0
> What are the platform(s), environment(s) and related component version(s)?
Android/IOS/Windows (probably all platforms, but verified on these)

> What is the use case or problem?
A deadlock can occur.

> What is the expected outcome?
No deadlock possibility.

> What is the actual outcome?
In our case a deadlock.

> What is the stacktrace of the exception(s) if any?
There is no exception.

> Do you have a code snippet or project that reproduces the problem?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.