[Java.Interop] Check for disposed state in JniValueManager methods

[radekdoulik]

Catched by Gendarme's
https://github.com/spouliot/gendarme/wiki/Gendarme.Rules.Exceptions.UseObjectDisposedExceptionRule(2.10)

[jonpryor]

Normally disposed is not protected, and I don't believe that it needs to be. MonoRuntimeValueManager can use RegisteredInstances instead.

[radekdoulik]

OK, how about replacing it with public bool Disposed { get; private set; } = false; property? That shouldn't do any harm.

[radekdoulik]

Or maybe protected IsDisposed as here https://docs.microsoft.com/en-us/dotnet/api/system.servicemodel.channels.message.isdisposed?view=netcore-2.0

As the Disposed name is used for events in .NET frameworks.

[jonpryor]

an IsDisposed property won't do "harm" per-se, but I don't like it, and it is generally not something that is provided.

See for example:

• System.IO.Stream
• System.Security.SecureString
• System.Threading.Timer
• etc.

In fact, I can (easily!) find only one place where an (Is)?Disposed-like property is present:

$ pwd
…/mono/mcs/class/corlib
$ git grep -l IDisposable System* | xargs grep Disposed
System.IO.IsolatedStorage/IsolatedStorageFile.cs:		internal bool IsDisposed {
System.IO.IsolatedStorage/IsolatedStorageFile.cs:				throw new ObjectDisposedException ("IsolatedStorageFile");
System.Security/SecureString.cs:					throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");
System.Security/SecureString.cs:				throw new ObjectDisposedException ("SecureString");

Note that it's internal, and all the others are not members.

What about corefx?

$ pwd
…/mono/external/corefx
$ git grep -l IDisposable src | xargs grep 'public.*Disposed' | grep -v tests/
src/System.ComponentModel.Primitives/ref/System.ComponentModel.Primitives.cs:        public event System.EventHandler Disposed { add { } remove { } }
src/System.ComponentModel.TypeConverter/ref/System.ComponentModel.cs:        public event System.EventHandler Disposed { add { } remove { } }
src/System.Runtime/ref/System.Runtime.cs:    public partial class ObjectDisposedException : System.InvalidOperationException
src/System.Runtime/ref/System.Runtime.cs:        public ObjectDisposedException(string objectName) { }
src/System.Runtime/ref/System.Runtime.cs:        public ObjectDisposedException(string message, System.Exception innerException) { }
src/System.Runtime/ref/System.Runtime.cs:        public ObjectDisposedException(string objectName, string message) { }

Two public events, not properties. Looking for protected members is no better.

TL;DR: Providing public/protected APIs to query the "disposed" status of an instance is not common. Providing such a method is decidedly uncommon, and I don't see any need to provide such an API here.

[radekdoulik]

It is not uncommon in the .NET framework. See https://docs.microsoft.com/en-us/dotnet/api/?view=netframework-4.7&term=IsDisposed

Some of these are abstract classes implementing IDisposable iface, which provide protected IsDisposed property. I think it makes sense to provide a single place to check for disposed status in this case, where we provide the dispose infrastructure as part of the abstraction.

[jonpryor]

Nice search URL; I wasn't aware of that.

That said, it finds 14 matches, most of which are System.Windows (7), System.Xaml (2), and System.Printing (2), leaving 3 in a nominally "core" namespace (System.Runtime, System.ServiceModel).

This is not what I'd call "not uncommon."

Alas, searching for IDisposable isn't entirely useful, as most matches are for explicitly implemented interface methods (bleh), so I guess searching for Dispose is the "better" bet. After hitting Load more results a lot -- and still not hitting any "end" -- there are (at least!) 213 matches for Dispose() Method.

Compare to 14.

This is not what I'd call "not uncommon"; it is very uncommon.

[radekdoulik]

Most of these 213 matches are not abstract classes implementing IDisposable. So I would still say it is not uncommon, to clarify, for abstract classes.

It is not that important though, so I will update the PR and move along.