NatsConnection does not implement the IAsyncDisposable Pattern Correctly.

[WallucePinkham]

Observed behavior

There is no way for me to call NatsConnection.Dispose() in my Dispose() method.

Expected behavior

I expected to be able to call Dispose() on a NatsConnection instance in my class' Dispose() method.

I expect this pattern to be followed when implementing IAsyncDisposable. The documentation suggests that IDisposable is also implemented.
https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync#the-disposeasync-method

Server and client version

N/A
This is an issue with the C# library implementation

Host environment

N/A
This is an issue with the C# library implementation

Steps to reproduce

This is an issue with the C# library implementation

[mtmk]

I guess it needs to trickle down to SubscriptionManager and NatsSubBase as well.

[rickdotnet]

To add some thoughts, I wouldn't go as far to say that it's not implemented "correctly" simply for not implementing IDisposable.

A good implementation pattern of the IAsyncDisposable interface is to be prepared for either synchronous or asynchronous disposal, however, it's not a requirement. If no synchronous disposable of your class is possible, having only IAsyncDisposable is acceptable.

Further in the same page they even address it by using the word "may". "You may need to implement both the IDisposable and IAsyncDisposable interfaces, especially when your class scope contains instances of these implementations".

That said, based on the section below it may be worth marking the class as sealed or delegating the dispose work to a virtual DisposeAsyncCore.

All nonsealed classes should be considered a potential base class, because they could be inherited. If you implement the async dispose pattern for any potential base class, you must provide the protected virtual ValueTask DisposeAsyncCore() method.

[mtmk]

I wouldn't go as far to say that it's not implemented "correctly" simply for not implementing IDisposable.

That is a good point.

@WallucePinkham do you have a case where you need an IDisposable instance considering we only support async calls?

[mnmr]

I took a quick look at the dispose logic, and I don't think it's a good fit for a synchronous Dispose method. The call chain includes calls to UnsubscribeAsync and other internal logic (which in itself is rather nasty, as it makes the dispose order important), but with all the things happening you'll likely want to wait for it to be done (which rules out just discarding the final ValueTask in a sync Dispose implementation). Instead, logic will have to be replicated as sync methods or sync-wait is needed, neither of which is appealing.

[mtmk]

Closing this issue because the synchronous dispose pattern is not needed right now. If anyone has more questions or reasons why we should, feel free to bring it up again.