Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Special case Disposing DotNetObjectReferences #2176

Merged
merged 3 commits into from
Aug 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ module DotNet {
}
}

function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, args: any[]): Promise<T> {
function invokePossibleInstanceMethodAsync<T>(assemblyName: string | null, methodIdentifier: string, dotNetObjectId: number | null, ...args: any[]): Promise<T> {
if (assemblyName && dotNetObjectId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just hardening? Or was there a bug here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just hardening. I was confused looking at the function name, because I assumed it only called instance methods.

throw new Error(`For instance method calls, assemblyName should be null. Received '${assemblyName}'.`) ;
javiercn marked this conversation as resolved.
Show resolved Hide resolved
}

const asyncCallId = nextAsyncCallId++;
const resultPromise = new Promise<T>((resolve, reject) => {
pendingAsyncCalls[asyncCallId] = { resolve, reject };
Expand Down Expand Up @@ -269,10 +273,7 @@ module DotNet {
}

public dispose() {
const promise = invokeMethodAsync<any>(
'Microsoft.JSInterop',
'DotNetDispatcher.ReleaseDotNetObject',
this._id);
const promise = invokePossibleInstanceMethodAsync<any>(null, '__Dispose', this._id);
promise.catch(error => console.error(error));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ public static partial class DotNetDispatcher
public static void BeginInvoke(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
public static void EndInvoke(string arguments) { }
public static string Invoke(string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
[Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
public static void ReleaseDotNetObject(long dotNetObjectId) { }
}
public static partial class DotNetObjectRef
{
Expand All @@ -18,7 +16,7 @@ public static partial class DotNetObjectRef
public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class
{
internal DotNetObjectRef() { }
public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😲

public TValue Value { get { throw null; } }
public void Dispose() { }
}
public partial interface IJSInProcessRuntime : Microsoft.JSInterop.IJSRuntime
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ public static partial class DotNetDispatcher
public static void BeginInvoke(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { }
public static void EndInvoke(string arguments) { }
public static string Invoke(string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson) { throw null; }
[Microsoft.JSInterop.JSInvokableAttribute("DotNetDispatcher.ReleaseDotNetObject")]
public static void ReleaseDotNetObject(long dotNetObjectId) { }
}
public static partial class DotNetObjectRef
{
Expand All @@ -18,7 +16,7 @@ public static partial class DotNetObjectRef
public sealed partial class DotNetObjectRef<TValue> : System.IDisposable where TValue : class
{
internal DotNetObjectRef() { }
public TValue Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public TValue Value { get { throw null; } }
public void Dispose() { }
}
public partial interface IJSInProcessRuntime : Microsoft.JSInterop.IJSRuntime
Expand Down
45 changes: 19 additions & 26 deletions src/JSInterop/Microsoft.JSInterop/src/DotNetDispatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace Microsoft.JSInterop
/// </summary>
public static class DotNetDispatcher
{
private const string DisposeDotNetObjectReferenceMethodName = "__Dispose";
internal static readonly JsonEncodedText DotNetObjectRefKey = JsonEncodedText.Encode("__dotNetObject");

private static readonly ConcurrentDictionary<AssemblyKey, IReadOnlyDictionary<string, (MethodInfo, Type[])>> _cachedMethodsByAssembly
Expand All @@ -38,7 +39,7 @@ public static string Invoke(string assemblyName, string methodIdentifier, long d
// the targeted method has [JSInvokable]. It is not itself subject to that restriction,
// because there would be nobody to police that. This method *is* the police.

var targetInstance = (object)null;
IDotNetObjectRef targetInstance = default;
if (dotNetObjectId != default)
{
targetInstance = DotNetObjectRefManager.Current.FindDotNetObject(dotNetObjectId);
Expand Down Expand Up @@ -78,7 +79,7 @@ public static void BeginInvoke(string callId, string assemblyName, string method
// original stack traces.
object syncResult = null;
ExceptionDispatchInfo syncException = null;
object targetInstance = null;
IDotNetObjectRef targetInstance = null;

try
{
Expand Down Expand Up @@ -127,21 +128,28 @@ public static void BeginInvoke(string callId, string assemblyName, string method
}
}

private static object InvokeSynchronously(string assemblyName, string methodIdentifier, object targetInstance, string argsJson)
private static object InvokeSynchronously(string assemblyName, string methodIdentifier, IDotNetObjectRef objectReference, string argsJson)
{
AssemblyKey assemblyKey;
if (targetInstance != null)
if (objectReference is null)
{
assemblyKey = new AssemblyKey(assemblyName);
}
else
{
if (assemblyName != null)
{
throw new ArgumentException($"For instance method calls, '{nameof(assemblyName)}' should be null. Value received: '{assemblyName}'.");
}

assemblyKey = new AssemblyKey(targetInstance.GetType().Assembly);
}
else
{
assemblyKey = new AssemblyKey(assemblyName);
if (string.Equals(DisposeDotNetObjectReferenceMethodName, methodIdentifier, StringComparison.Ordinal))
{
// The client executed dotNetObjectReference.dispose(). Dispose the reference and exit.
objectReference.Dispose();
return default;
}
pranavkm marked this conversation as resolved.
Show resolved Hide resolved

assemblyKey = new AssemblyKey(objectReference.Value.GetType().Assembly);
}

var (methodInfo, parameterTypes) = GetCachedMethodInfo(assemblyKey, methodIdentifier);
Expand All @@ -150,7 +158,8 @@ private static object InvokeSynchronously(string assemblyName, string methodIden

try
{
return methodInfo.Invoke(targetInstance, suppliedArgs);
// objectReference will be null if this call invokes a static JSInvokable method.
return methodInfo.Invoke(objectReference?.Value, suppliedArgs);
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
}
catch (TargetInvocationException tie) // Avoid using exception filters for AOT runtime support
{
Expand Down Expand Up @@ -280,22 +289,6 @@ internal static void ParseEndInvokeArguments(JSRuntimeBase jsRuntimeBase, string
}
}

/// <summary>
/// Releases the reference to the specified .NET object. This allows the .NET runtime
/// to garbage collect that object if there are no other references to it.
///
/// To avoid leaking memory, the JavaScript side code must call this for every .NET
/// object it obtains a reference to. The exception is if that object is used for
/// the entire lifetime of a given user's session, in which case it is released
/// automatically when the JavaScript runtime is disposed.
/// </summary>
/// <param name="dotNetObjectId">The identifier previously passed to JavaScript code.</param>
[JSInvokable(nameof(DotNetDispatcher) + "." + nameof(ReleaseDotNetObject))]
public static void ReleaseDotNetObject(long dotNetObjectId)
{
DotNetObjectRefManager.Current.ReleaseDotNetObject(dotNetObjectId);
}

private static (MethodInfo, Type[]) GetCachedMethodInfo(AssemblyKey assemblyKey, string methodIdentifier)
{
if (string.IsNullOrWhiteSpace(assemblyKey.AssemblyName))
Expand Down
3 changes: 1 addition & 2 deletions src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRef.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ public static class DotNetObjectRef
/// <returns>An instance of <see cref="DotNetObjectRef{TValue}" />.</returns>
public static DotNetObjectRef<TValue> Create<TValue>(TValue value) where TValue : class
{
var objectId = DotNetObjectRefManager.Current.TrackObject(value);
return new DotNetObjectRef<TValue>(objectId, value);
return new DotNetObjectRef<TValue>(DotNetObjectRefManager.Current, value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to fight with your other change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. But it's easier to review these things piecemeal.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.JSInterop
internal class DotNetObjectRefManager
{
private long _nextId = 0; // 0 signals no object, but we increment prior to assignment. The first tracked object should have id 1
private readonly ConcurrentDictionary<long, object> _trackedRefsById = new ConcurrentDictionary<long, object>();
private readonly ConcurrentDictionary<long, IDotNetObjectRef> _trackedRefsById = new ConcurrentDictionary<long, IDotNetObjectRef>();

public static DotNetObjectRefManager Current
{
Expand All @@ -25,15 +25,15 @@ public static DotNetObjectRefManager Current
}
}

public long TrackObject(object dotNetObjectRef)
public long TrackObject(IDotNetObjectRef dotNetObjectRef)
{
var dotNetObjectId = Interlocked.Increment(ref _nextId);
_trackedRefsById[dotNetObjectId] = dotNetObjectRef;

return dotNetObjectId;
}

public object FindDotNetObject(long dotNetObjectId)
public IDotNetObjectRef FindDotNetObject(long dotNetObjectId)
{
return _trackedRefsById.TryGetValue(dotNetObjectId, out var dotNetObjectRef)
? dotNetObjectRef
Expand Down
56 changes: 49 additions & 7 deletions src/JSInterop/Microsoft.JSInterop/src/DotNetObjectRefOfT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,53 @@ namespace Microsoft.JSInterop
[JsonConverter(typeof(DotNetObjectReferenceJsonConverterFactory))]
public sealed class DotNetObjectRef<TValue> : IDotNetObjectRef, IDisposable where TValue : class
{
private readonly DotNetObjectRefManager _referenceManager;
private readonly TValue _value;
private readonly long _objectId;

/// <summary>
/// Initializes a new instance of <see cref="DotNetObjectRef{TValue}" />.
/// </summary>
/// <param name="objectId">The object Id.</param>
/// <param name="referenceManager"></param>
/// <param name="value">The value to pass by reference.</param>
internal DotNetObjectRef(long objectId, TValue value)
internal DotNetObjectRef(DotNetObjectRefManager referenceManager, TValue value)
{
_referenceManager = referenceManager;
_objectId = _referenceManager.TrackObject(this);
_value = value;
}

internal DotNetObjectRef(DotNetObjectRefManager referenceManager, long objectId, TValue value)
{
ObjectId = objectId;
Value = value;
_referenceManager = referenceManager;
_objectId = objectId;
_value = value;
}

/// <summary>
/// Gets the object instance represented by this wrapper.
/// </summary>
public TValue Value { get; }
public TValue Value
{
get
{
ThrowIfDisposed();
return _value;
}
}

internal long ObjectId { get; }
internal long ObjectId
{
get
{
ThrowIfDisposed();
return _objectId;
}
}

object IDotNetObjectRef.Value => Value;

internal bool Disposed { get; private set; }

/// <summary>
/// Stops tracking this object reference, allowing it to be garbage collected
Expand All @@ -41,7 +71,19 @@ internal DotNetObjectRef(long objectId, TValue value)
/// </summary>
public void Dispose()
{
DotNetObjectRefManager.Current.ReleaseDotNetObject(ObjectId);
if (!Disposed)
{
Disposed = true;
_referenceManager.ReleaseDotNetObject(_objectId);
}
}

private void ThrowIfDisposed()
{
if (Disposed)
{
throw new ObjectDisposedException(GetType().Name);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ public override DotNetObjectRef<TValue> Read(ref Utf8JsonReader reader, Type typ
throw new JsonException($"Required property {DotNetObjectRefKey} not found.");
}

var value = (TValue)DotNetObjectRefManager.Current.FindDotNetObject(dotNetObjectId);
return new DotNetObjectRef<TValue>(dotNetObjectId, value);
var referenceManager = DotNetObjectRefManager.Current;
return (DotNetObjectRef<TValue>)referenceManager.FindDotNetObject(dotNetObjectId);
}

public override void Write(Utf8JsonWriter writer, DotNetObjectRef<TValue> value, JsonSerializerOptions options)
Expand Down
1 change: 1 addition & 0 deletions src/JSInterop/Microsoft.JSInterop/src/IDotNetObjectRef.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ namespace Microsoft.JSInterop
{
internal interface IDotNetObjectRef : IDisposable
{
object Value { get; }
}
}
20 changes: 17 additions & 3 deletions src/JSInterop/Microsoft.JSInterop/test/DotNetDispatcherTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public Task CanInvokeStaticWithParams() => WithJSRuntime(jsRuntime =>
Assert.False(resultDto2Ref.TryGetProperty(nameof(TestDTO.IntVal), out _));

Assert.True(resultDto2Ref.TryGetProperty(DotNetDispatcher.DotNetObjectRefKey.EncodedUtf8Bytes, out var property));
var resultDto2 = Assert.IsType<TestDTO>(DotNetObjectRefManager.Current.FindDotNetObject(property.GetInt64()));
var resultDto2 = Assert.IsType<DotNetObjectRef<TestDTO>>(DotNetObjectRefManager.Current.FindDotNetObject(property.GetInt64())).Value;
Assert.Equal("MY STRING", resultDto2.StringVal);
Assert.Equal(1299, resultDto2.IntVal);
});
Expand Down Expand Up @@ -202,6 +202,20 @@ public Task CanInvokeBaseInstanceVoidMethod() => WithJSRuntime(jsRuntime =>
Assert.True(targetInstance.DidInvokeMyBaseClassInvocableInstanceVoid);
});

[Fact]
public Task DotNetObjectReferencesCanBeDisposed() => WithJSRuntime(jsRuntime =>
{
// Arrange
var targetInstance = new SomePublicType();
var objectRef = DotNetObjectRef.Create(targetInstance);

// Act
DotNetDispatcher.BeginInvoke(null, null, "__Dispose", objectRef.ObjectId, null);

// Assert
Assert.True(objectRef.Disposed);
});

[Fact]
public Task CannotUseDotNetObjectRefAfterDisposal() => WithJSRuntime(jsRuntime =>
{
Expand Down Expand Up @@ -230,7 +244,7 @@ public Task CannotUseDotNetObjectRefAfterReleaseDotNetObject() => WithJSRuntime(
var targetInstance = new SomePublicType();
var objectRef = DotNetObjectRef.Create(targetInstance);
jsRuntime.Invoke<object>("unimportant", objectRef);
DotNetDispatcher.ReleaseDotNetObject(1);
objectRef.Dispose();

// Act/Assert
var ex = Assert.Throws<ArgumentException>(
Expand Down Expand Up @@ -320,7 +334,7 @@ public Task CanInvokeInstanceMethodWithParams() => WithJSRuntime(jsRuntime =>

// Assert
Assert.Equal("[\"You passed myvalue\",{\"__dotNetObject\":3}]", resultJson);
var resultDto = (TestDTO)jsRuntime.ObjectRefManager.FindDotNetObject(3);
var resultDto = ((DotNetObjectRef<TestDTO>)jsRuntime.ObjectRefManager.FindDotNetObject(3)).Value;
Assert.Equal(1235, resultDto.IntVal);
Assert.Equal("MY STRING", resultDto.StringVal);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ public Task NotifiesAssociatedJsRuntimeOfDisposal() => WithJSRuntime(jsRuntime =
var objRef = DotNetObjectRef.Create(new object());

// Act
Assert.Equal(1, objRef.ObjectId);
objRef.Dispose();

// Assert
var ex = Assert.Throws<ArgumentException>(() => jsRuntime.ObjectRefManager.FindDotNetObject(objRef.ObjectId));
var ex = Assert.Throws<ArgumentException>(() => jsRuntime.ObjectRefManager.FindDotNetObject(1));
Assert.StartsWith("There is no tracked object with id '1'.", ex.Message);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ public void SerializesDotNetObjectWrappersInKnownFormat()
Assert.Equal("[{\"__dotNetObject\":1},{\"obj2\":{\"__dotNetObject\":2},\"obj3\":{\"__dotNetObject\":3}}]", call.ArgsJson);

// Assert: Objects were tracked
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1));
Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(2));
Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(3));
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1).Value);
Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(2).Value);
Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(3).Value);
}

[Fact]
Expand Down
8 changes: 4 additions & 4 deletions src/JSInterop/Microsoft.JSInterop/test/JSRuntimeBaseTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,10 @@ public void SerializesDotNetObjectWrappersInKnownFormat()
Assert.Equal("[{\"__dotNetObject\":1},{\"obj2\":{\"__dotNetObject\":3},\"obj3\":{\"__dotNetObject\":4},\"obj1SameRef\":{\"__dotNetObject\":1},\"obj1DifferentRef\":{\"__dotNetObject\":2}}]", call.ArgsJson);

// Assert: Objects were tracked
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1));
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(2));
Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(3));
Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(4));
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(1).Value);
Assert.Same(obj1, runtime.ObjectRefManager.FindDotNetObject(2).Value);
Assert.Same(obj2, runtime.ObjectRefManager.FindDotNetObject(3).Value);
Assert.Same(obj3, runtime.ObjectRefManager.FindDotNetObject(4).Value);
}

[Fact]
Expand Down