Skip to content

Don't throw ObjectDisposedException when disposing Observer for the first time - #219

Closed
affederaffe wants to merge 2 commits into
tmds:mainfrom
affederaffe:fix/dispose-observer
Closed

Don't throw ObjectDisposedException when disposing Observer for the first time#219
affederaffe wants to merge 2 commits into
tmds:mainfrom
affederaffe:fix/dispose-observer

Conversation

@affederaffe

Copy link
Copy Markdown
Contributor

When disposing Observers, an ObjectDisposedException was thrown event though it was the first time disposing said observer. This PR fixes the behavior to only throw an exception when trying to dispose multiple times.

@kekekeks

Copy link
Copy Markdown

Note that by convention it's OK to call Dispose multiple times and it should never throw. I. e. Dispose is supposed to be a method that's always safe to call.

From https://learn.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?view=net-8.0:

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times. Instance methods other than Dispose can throw an ObjectDisposedException when resources are already disposed.

@tmds

tmds commented Jan 17, 2024

Copy link
Copy Markdown
Owner

Can you share some code that reproduces the problem?

@tmds

tmds commented Jan 17, 2024

Copy link
Copy Markdown
Owner

Note that by convention it's OK to call Dispose multiple times and it should never throw. I. e. Dispose is supposed to be a method that's always safe to call.

afaik Disposing an Observer never throws.

It does call the observer's action handler with an ObjectDisposedException.
This is intentional, and is meant to inform the consumer the stream stopped similar to an RX "IObserver.OnError/OnCompleted".

@affederaffe

Copy link
Copy Markdown
Contributor Author

In my opinion that's a bad design per convention it should not be an Exception.
Since the owner of the Observer explicitly (or via using) disposes the it, so we already know that it "completed"

@tmds

tmds commented Jan 17, 2024

Copy link
Copy Markdown
Owner

In my opinion that's a bad design per convention it should not be an Exception.

I agree that it may be non-intuitive that this is done with an exception.

An other, more explicit parameter on the action delegate would have been clearer.

Since the owner of the Observer explicitly (or via using) disposes the it, so we already know that it "completed"

Depending on how you structure your code, it may be convenient to be able handle the end of the stream in the handler. That is the rationale for invoking the handler.

I'll close this as I think there is meaning in notifying the observer the stream ended through disposal, and I don't want to introduce a breaking change.

I'm open to an API addition which has the semantics you want. If you have a proposal, please create an issue for it.

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.

3 participants