change the timing to remove PropertyChanged event

[takytank]

Summary for English Speakers:

This issue addresses a problem with the ReactivePropertySlim class in the ReactiveProperty library. When updating from version 8.2.0 to 9.0.0, an InvalidOperationException occurs in the PropertyChanged event after the object is disposed. The proposed solution changes the timing of the event subscription removal to avoid this exception. The event is now unsubscribed before the OnCompleted method is executed, ensuring a more natural order and preventing exceptions caused by unrelated property change notifications.

---

いつもお世話になっております。

ReactiveProperty を 8.2.0 から 9.0.0 以降にアップデートすると、ReactivePropertySlim 周りでたまに InvalidOperationException が発生する事があるという問題が手元で起こっています。
例外の発生箇所は、SimplePropertyObservable.Subscription.PropertyChanged 関数でした。

private void PropertyChanged(object? sender, PropertyChangedEventArgs e)
{
	if (_isDisposed) throw new InvalidOperationException();
	if (e.PropertyName != _propertyName && !string.IsNullOrEmpty(e.PropertyName)) return;

	try
	{
		_observer.OnNext(_accessor(_subject));
	}
	catch (Exception ex)
	{
		_onError = true;
		_observer.OnError(ex);
		Dispose();
	}
}

例外発生時、確かに _isDisposed は true でした。また、e.PropertyName と _propertyName は別の名前でした。

しかし、本来、Dispose 関数が呼び出されて _isDisposed が true になっている場合、PropertyChanged イベントも remove されているはずで、PropertyChanged 関数は呼び出されないはずです。
おそらく、_isDisposed が true になってからイベントが購読解除されるまでの OnCompleted の処理中にイベントが発生してしまっていると推測します。
これをなるべく避けるために、イベントの購読解除を OnCompleted よりも先に行うように変更しました。OnCompleted の実行前に OnNext が走らないようにするのは、むしろ自然な順序かと思います。

加えて、今回起こっている問題の場合、発火されたプロパティ変更通知は Dispose された Subscription オブジェクト宛てのものではありませんでした。無関係の変更通知で例外を発生させるのは変な挙動かなと思います。
そのため、_isDisposed のチェックとプロパティ名のチェック順を逆にしました。

issue を立てずにいきなりプルリクエストを送る形になってしまいましたが、一考して頂けると幸いです。

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

Source/ReactiveProperty.Core/Internals/SimplePropertyObservable.cs:64

• The _isDisposed check was moved after the property name check. Ensure that this change does not allow unintended behavior when _isDisposed is true but the property name matches _propertyName.

if (_isDisposed) throw new InvalidOperationException();

[runceel]

@takytank ありがとうございます。マージして v9.7.0-pre1 でリリースしました。
念のため改善されているか確認していただいてもいいですか？
https://www.nuget.org/packages/ReactiveProperty/9.7.0-pre1

[takytank]

マージありがとうございます。

ただ、v9.7.0-pre1 を試して見たところ、残念ながら完全には解決していないようでした……
e.PropertyName と _propertyName が同じで、_isDisposed が true のケースもあるようです。

InvalidOperationException を投げるのではなく、単に return するというのはまずいでしょうか？

[runceel]

@takytank
#501 の変更を加えて v9.7.0-pre2 を出してみました。
今リリース用のワークフローが動いているので、もうすぐ NuGet に公開されると思います。
こちらだとどうでしょうか？

[takytank]

@runceel
v9.7.0-pre2 では、無事に問題が発生しなくなりました。
修正ありがとうございます。

[runceel]

@takytank v9.7.0 として同じものをリリースするワークフローを起動したので、しばらくしたら NuGet に出てくると思います。

[takytank]

迅速なリリースに感謝します……！