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.

Sign up for GitHub

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

Bypasses deserialization of return value when 'InvokeVoidAsync' is used. #22056

Closed
wants to merge 8 commits into from
Closed

Bypasses deserialization of return value when 'InvokeVoidAsync' is used. #22056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
defef94
Bypasses deserialization of return value when 'InvokeVoidAsync' is used.
May 20, 2020
ca19908
Set TaskCompletionSource result to 'null' for instances where no valu…
Jun 15, 2020
711851c
Merge branch 'master' into invokeVoidAsync-fix
StevenRasmussen Jul 9, 2020
0ebab51
Remove 'InvokeVoidAsync' method from JSRuntime and intstead call 'Inv…
StevenRasmussen Jul 9, 2020
68c41ab
Update JsInProcessRuntimeExtensions to use VoidReturn for 'InvokeVoid…
StevenRasmussen Jul 9, 2020
2691bf1
Add new 'treatReturnAsVoid' parameter to 'invokeJSFromDotNet' method.
StevenRasmussen Jul 9, 2020
8f8d165
Added 'treatReturnAsVoid' to InvokeJS method.
StevenRasmussen Jul 9, 2020
e7e0cdb
Updated methods to pass allong new 'treatReturnAsVoid' parameter.
StevenRasmussen Jul 10, 2020
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
27 changes: 24 additions & 3 deletions src/JSInterop/Microsoft.JSInterop/src/JSRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,20 @@ public ValueTask<TValue> InvokeAsync<TValue>(string identifier, CancellationToke
}
}

/// <summary>
/// Invokes the specified JavaScript function asynchronously.
/// </summary>
/// <param name="identifier">An identifier for the function to invoke. For example, the value <c>"someScope.someFunction"</c> will invoke the function <c>window.someScope.someFunction</c>.</param>
/// <param name="cancellationToken">
/// A cancellation token to signal the cancellation of the operation. Specifying this parameter will override any default cancellations such as due to timeouts
/// (<see cref="JSRuntime.DefaultAsyncTimeout"/>) from being applied.
/// </param>
/// <param name="args">JSON-serializable arguments.</param>
public async ValueTask InvokeVoidAsync(string identifier, CancellationToken cancellationToken, object[] args)
{
await this.InvokeAsync<VoidReturn>(identifier, cancellationToken, args);
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest, is it possible to use System.Void instead of VoidReturn?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't appear that we can use System.Void directly:
image

Copy link
Author

Choose a reason for hiding this comment

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

We could look at using reflection to invoke the method if you like... something like this:

public async ValueTask InvokeVoidAsync(string identifier, CancellationToken cancellationToken, object[] args)
{
    var voidType = typeof(void);
    var invokeAsyncMethod = this.GetType().GetMethod(nameof(InvokeAsync), new Type[] { typeof(string), typeof(CancellationToken), typeof(object[]) }).MakeGenericMethod(voidType);
    var task = (Task)invokeAsyncMethod.Invoke(this, new object[] { identifier, cancellationToken, args });
    await task;
}

This compiles but I haven't actually tested the code to see if it works. If you think it's worth pursuing I can look at getting my environment setup to test and/or create a unit test if you can point me in the right direction as to where those are. I've just been opening up the project directly since the JSInterop.slnf file doesn't work and I'm not actually sure what solution uses this project.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid reflection here if we can. Your previous solution was fine really!

}

private void CleanupTasksAndRegistrations(long taskId)
{
_pendingTasks.TryRemove(taskId, out _);
Expand Down Expand Up @@ -172,9 +186,11 @@ internal void EndInvokeJS(long taskId, bool succeeded, ref Utf8JsonReader jsonRe
if (succeeded)
{
var resultType = TaskGenericsUtil.GetTaskCompletionSourceResultType(tcs);

var result = JsonSerializer.Deserialize(ref jsonReader, resultType, JsonSerializerOptions);
TaskGenericsUtil.SetTaskCompletionSourceResult(tcs, result);
if (resultType != typeof(VoidReturn))
{
var result = JsonSerializer.Deserialize(ref jsonReader, resultType, JsonSerializerOptions);
TaskGenericsUtil.SetTaskCompletionSourceResult(tcs, result);
}
Copy link

Choose a reason for hiding this comment

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

Curious, is there a downside to leaving tcs hanging here if resultType is VoidReturn?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes we definitely do not want to leave tcs hanging. This would prevent the caller from knowing when their call had completed.

Seems like we should be setting the result to null since that's probably the best representation of the idea of "no value".

Copy link
Author

Choose a reason for hiding this comment

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

@watfordgnf - Nice catch! @SteveSandersonMS - Fixed.

}
else
{
Expand Down Expand Up @@ -231,5 +247,10 @@ internal IDotNetObjectReference GetObjectReference(long dotNetObjectId)
/// </summary>
/// <param name="dotNetObjectId">The ID of the <see cref="DotNetObjectReference{TValue}"/>.</param>
internal void ReleaseObjectReference(long dotNetObjectId) => _trackedRefsById.TryRemove(dotNetObjectId, out _);

/// <summary>
/// Used as a type for the <see cref="TaskCompletionSource{TResult}"/> that represents nothing should be returned.
/// </summary>
private class VoidReturn { }
}
}
6 changes: 3 additions & 3 deletions src/JSInterop/Microsoft.JSInterop/src/JSRuntimeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static async ValueTask InvokeVoidAsync(this IJSRuntime jsRuntime, string
throw new ArgumentNullException(nameof(jsRuntime));
}

await jsRuntime.InvokeAsync<object>(identifier, args);
await jsRuntime.InvokeVoidAsync(identifier, args);
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods are currently calling themselves which results in infinite recursion.
I think this also needs to be exposed on the IJSRuntime interface or use VoidReturn here and make it internal / public.

Copy link
Author

Choose a reason for hiding this comment

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

Doh! You're right. I thought about adding it to the interface as well but didn't since the current implementation just used extension methods. I think it makes sense to add it to the interface at this point... @SteveSandersonMS thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Hearing no response I went ahead and added the methods to the interface. @SteveSandersonMS is there anything else you are looking for/need for this PR to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for following up on this. I know this PR has been delayed for a long time due to slow reviews on our side, so I'd be keen to work with you and get this merged in the next few days or next week, depending on what works best for you.

Apart from my suggestion above about avoiding the breaking change, the main remaining thing would be adding a few suitable test cases into the test project. Is that something you feel able to handle?

}

/// <summary>
Expand Down Expand Up @@ -91,7 +91,7 @@ public static async ValueTask InvokeVoidAsync(this IJSRuntime jsRuntime, string
throw new ArgumentNullException(nameof(jsRuntime));
}

await jsRuntime.InvokeAsync<object>(identifier, cancellationToken, args);
await jsRuntime.InvokeVoidAsync(identifier, cancellationToken, args);
}

/// <summary>
Expand Down Expand Up @@ -134,7 +134,7 @@ public static async ValueTask InvokeVoidAsync(this IJSRuntime jsRuntime, string
using var cancellationTokenSource = timeout == Timeout.InfiniteTimeSpan ? null : new CancellationTokenSource(timeout);
var cancellationToken = cancellationTokenSource?.Token ?? CancellationToken.None;

await jsRuntime.InvokeAsync<object>(identifier, cancellationToken, args);
await jsRuntime.InvokeVoidAsync(identifier, cancellationToken, args);
}
}
}