[Mono.Android.dll] Fix [SupportedOSPlatform] warnings. #8247
Conversation
jpobst
force-pushed
the
supported-os-warnings
branch
from
ec203a4 to
8c1ffe8
Compare
| @@ -170,7 +170,7 @@ public void Put (string key, int value) | |||
| id_put_Ljava_lang_String_Ljava_lang_Integer_ = JNIEnv.GetMethodID (class_ref, "put", "(Ljava/lang/String;Ljava/lang/Integer;)V"); | |||
| IntPtr jkey = JNIEnv.NewString (key); | |||
| try { | |||
| using (var val = new Java.Lang.Integer (value)) | |||
| using (var val = Java.Lang.Integer.ValueOf (value)) | |||
There was a problem hiding this comment.
Using Integer.valueOf() may be the recommended alternative, but…
Our problem is going to be "instance sharing": Integer.valueOf() "yield[s] significantly better space and time performance" because it has a cache:
This presents the possibility of "accidental instance sharing". What happens if two threads run:
var v = Java.Lang.Integer.ValueOf(42);What happens is that Integer.valueOf(42) will return the same Java-side instance, so Integer.ValueOf() will (attempt) to return the same managed callable wrapper instance.
So far, so fine. But that using is what kills things; if two threads run:
using (var val = Java.Lang.Integer.ValueOf(42)) {
val.FloatValue();
}then we have a possible "single world ordering" of:
- Thread 1: calls
Integer.ValueOf(42), callsInteger.valueOf(42), wrapper instance created and registered. - Thread 1: calls
val.FloatValue() - Thread 2: calls
Integer.ValueOf(42), gets wrapper instance created in (1) - Thread 1: calls
val.Dispose() - Thread 2: calls
val.FloatValue().
Step (5) throws an System.ObjectDisposedException. Oops.
You can either have using (var v = new T(…)) or we can have var v = T.valueOf(…). We cannot intermix them, not safely.
(Of course, I have some ideas to make this "more reasonable", but they have not been finished yet.)
We thus have a question: which is "worse":
- the extra GREFs that will happen by using
Integer.ValueOf()without ausingblock, or - the extra Java-side (and C# side!)
Integerinstances which will be created by using the constructor?
That would be a fascinating microbenchmark to see the results of.
For now, I think we should stick with the current using approach and continue using the constructors.
There was a problem hiding this comment.
The same logic applies to all the other types.
| else if (type == typeof (string)) | ||
| return JNIEnv.NewString ((string)obj); | ||
| else if (typeof (IJavaObject).IsAssignableFrom (type)) | ||
| if (obj is bool bool_obj) |
There was a problem hiding this comment.
"Interestingly", I can't easily find any code which calls JavaObject.GetHandle(object). Maybe we could just remove it entirely?
| return ((IJavaObject)obj).Handle; | ||
| else | ||
| return new JavaObject (obj).Handle; | ||
| } | ||
|
|
||
| public static object? GetObject (IntPtr handle, JniHandleOwnership transfer) | ||
| { | ||
| Java.Lang.Object? jlo = Java.Lang.Object.GetObject (handle, transfer) as Java.Lang.Object; | ||
| if (jlo == null) | ||
| if (Java.Lang.Object.GetObject (handle, transfer) is not Java.Lang.Object jlo) |
There was a problem hiding this comment.
Similarly, what calls JavaObject.GetObject()? git grep JavaObject.GetObject only finds:
src/Mono.Android/Android.Runtime/JNIEnv.cs: var javaException = JavaObject.GetObject<Java.Lang.Throwable> (env, javaExceptionPtr, JniHandleOwnership.DoNotTransfer)!;
which is a generic method invocation, and thus can't be this method. Is this method actually used anymore? Can we just remove it?
| else if (jlo is Java.Lang.Double) | ||
| return Dispose ((Java.Lang.Double) jlo, v => v.DoubleValue ()); | ||
| else if (jlo is Java.Lang.Boolean bool_jlo) | ||
| return Dispose (bool_jlo, v => v.BooleanValue ()); |
There was a problem hiding this comment.
…I mostly care about who's calling this method so I can know about possible value sharing, a'la Integer.valueOf(). Is it safe to call Dispose()? Is this a ticking timebomb or just dead code?
| { typeof (long), value => new Java.Lang.Long ((long) value) }, | ||
| { typeof (float), value => new Java.Lang.Float ((float) value) }, | ||
| { typeof (double), value => new Java.Lang.Double ((double) value) }, | ||
| { typeof (bool), value => Java.Lang.Boolean.ValueOf ((bool) value) }, |
There was a problem hiding this comment.
The changes are probably safe, in that I can't easily tell if there are any callers that will be Dispose()ing of the value…
| @@ -308,39 +308,39 @@ static class JavaConvert { | |||
|
|
|||
| static Dictionary<Type, Func<object, IntPtr>> LocalJniHandleConverters = new Dictionary<Type, Func<object, IntPtr>> { | |||
| { typeof (bool), value => { | |||
| using (var v = new Java.Lang.Boolean ((bool) value)) | |||
| using (var v = Java.Lang.Boolean.ValueOf ((bool) value)) | |||
There was a problem hiding this comment.
Same accidental value sharing problem…
* main: [Mono.Android.dll] Fix [SupportedOSPlatform] warnings. (dotnet#8247) Bump to xamarin/monodroid@a5248591 (dotnet#8258) [Mono.Android] Bind `java.time` and `java.time.chrono` packages. (dotnet#8088) Bump to xamarin/Java.Interop/main@5adb4d4 (dotnet#8251) [ci] Use fewer agents for macOS MSBuild tests (dotnet#8257) [ci] Report when dotnet-test-slicer fails (dotnet#8259) [monodroid] Do not build host or classic runtimes (dotnet#8256)
Fixes #7590
Fix the remaining
[SupportedOSPlatform]warnings in manually bound code. Other issues we were previously seeing appear to have been a bug in earlier .NET 8 previews.Of particular note, the constructors for
Java.Lang.Boolean,Java.Lang.Integer, etc. have been marked asDeprecatedin API-33. (source).This PR replaces them with the suggested
valueOfmethods.@jonpryor Please verify this change is acceptable for our use case.