Skip to content

Fix: AsObservable immediately calls Dispose on completion - #392

Closed
TORISOUP wants to merge 1 commit into
Cysharp:mainfrom
TORISOUP:fix_AsObservable
Closed

Fix: AsObservable immediately calls Dispose on completion#392
TORISOUP wants to merge 1 commit into
Cysharp:mainfrom
TORISOUP:fix_AsObservable

Conversation

@TORISOUP

Copy link
Copy Markdown
Contributor

Resubmitting #331.

As pointed out in #330, when using AsObservable() in combination with message-delaying operators such as Delay or ObserveOn, the OnCompleted signal was not properly propagated downstream.

var subject = new Subject<Unit>();
var fakeFrameProvider = new FakeFrameProvider();

subject
    .AsObservable()
    .ObserveOn(fakeFrameProvider)
    .Subscribe(
        onNext: _ => Console.WriteLine("Next"),
        onCompleted: _ => Console.WriteLine("Completed")
    );

subject.OnNext(Unit.Default);
fakeFrameProvider.Advance();
subject.OnCompleted();
fakeFrameProvider.Advance();

// "Completed" is not printed

The cause was that WrappedObserver is defined with AutoDisposeOnCompleted = true, and AsObservable() uses it as-is.
As a fix, AsObservable() now uses a dedicated observer.

A similar issue was also present in AsSystemObservable(), so it has been fixed as well.

Copilot AI lite review requested due to automatic review settings August 26, 2026 12:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a completion-propagation bug when AsObservable() (and AsSystemObservable()) is used in operator chains that delay notifications, where OnCompleted could be lost due to premature disposal.

Changes:

  • Update AsObservable() to subscribe with a dedicated observer that does not auto-dispose on completion.
  • Update AsSystemObservable() bridge observer behavior (and add a new regression test for delayed completion on AsObservable()).
  • Add a unit test covering AsObservable() composed with DelayFrame(...) to ensure completion is delivered.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/R3.Tests/OperatorTests/AsObservableTest.cs Adds a regression test ensuring delayed completion is observed after AsObservable() + DelayFrame.
src/R3/Operators/AsObservable.cs Changes AsObservable() to avoid premature disposal on completion; adjusts AsSystemObservable() observer behavior.
Suppressed comments (1)

tests/R3.Tests/OperatorTests/AsObservableTest.cs:46

  • PR description says a similar completion-propagation issue was fixed in AsSystemObservable(), but the tests here only cover the basic (non-delayed) AsSystemObservable path. Please add a regression test that composes AsSystemObservable() with a message-delaying System.Reactive operator (e.g., Delay(TimeSpan.Zero) or ObserveOn(...)) and asserts OnCompleted is eventually observed downstream.
    [Fact]
    public void AsSystemObservable()
    {
        {
            var p = new Subject<int>();
            var l = new List<int>();
            Exception? ex = null;
            bool completed = false;
            p.AsSystemObservable().Subscribe(l.Add, e => ex = e, () => completed = true);


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 61 to 65
sealed class ObserverToObserver(IObserver<T> observer) : Observer<T>
{
protected override bool AutoDisposeOnCompleted => false;

protected override void OnNextCore(T value)
@TORISOUP

Copy link
Copy Markdown
Contributor Author

Closing this to explore a different approach.

@TORISOUP TORISOUP closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants