Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
2.27k stars 99 forks source link

System.InvalidOperationException : Disposable is already assigned. #247

Closed Bezarius closed 3 months ago

Bezarius commented 3 months ago
public class TestObservable<T> : Observable<T>, IDisposable
{
    private Subject<T> _subject = new Subject<T>();

    protected override IDisposable SubscribeCore(Observer<T> observer)
    {
        return _subject.Subscribe(observer);
    }

    public void Dispose()
    {
        _subject.Dispose();
    }
}
[Test]
public void TestCustomObservable()
{
    var entry = new object();
    var subject = new TestObservable<object>();
    IDisposable disposable = null;
    try
    {
        disposable = subject.Subscribe(x => // <== we get that exception: System.InvalidOperationException : Disposable is already assigned.
        {
            //Assert.That(x, Is.EqualTo(entry));
        });

    }
    finally
    {
        disposable?.Dispose();
    }
}

if we change TestObservable like this everyting will be fine

 public class TestObservable<T> : Subject<T>, IDisposable
 {

 }

Explanation:

public class TestObservable<T> : Observable<T>, IDisposable
{
    private Subject<T> _subject = new Subject<T>();

    protected override IDisposable SubscribeCore(Observer<T> observer)
    {
        var subscription = _subject.Subscribe(observer); // here first assignment
        return subscription; // on return, second assignment to same SingleAssignmentDisposableCore
    }

    public void Dispose()
    {
        _subject.Dispose();
    }
}

In my opinion, the code in test is quite legal. It seems to me that this case needs to be studied and a recommendation on how to work around this problem should be added to the documentation. At the moment, I have no idea how to solve the issue correctly. Using Subject as a base class is definitely not correct. There is a feeling that support for the IObservable\IObserver interfaces is still necessary.

I can't help but draw attention to the fact that there may potentially be cases where a developer may need a different base class, not Observable. In this case, dev will have to create a separate property for the subscription, which, in my opinion, will worsen the readability of the code.

Bezarius commented 3 months ago

Found temporal solution:

public class TestObservable<T> : Observable<T>, IDisposable
{
    private Subject<T> _subject = new Subject<T>();

    protected override IDisposable SubscribeCore(Observer<T> observer)
    {
        return ObservableSubscribeExtensions.Subscribe(_subject, observer.OnNext);
    }

    public void Dispose()
    {
        _subject.Dispose();
    }
}
neuecc commented 3 months ago

Observable is a base class for creating Observables, so it is inappropriate to delegate it in SubscribeCore. I admit that the lack of an interface is inconvenient, but I would like you to work around this by adding AsObservable, etc.