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

StevenRasmussen · 2020-05-20T21:43:54Z

When using the InvokeVoidAsync method, deserialization of the return object is not required. This bypasses the deserialization which currently causes an error to be thrown in this instance.

Addresses #16632

watfordgnf · 2020-06-08T21:51:56Z

src/JSInterop/Microsoft.JSInterop/src/JSRuntime.cs

+                    {
+                        var result = JsonSerializer.Deserialize(ref jsonReader, resultType, JsonSerializerOptions);
+                        TaskGenericsUtil.SetTaskCompletionSourceResult(tcs, result);
+                    }


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

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".

@watfordgnf - Nice catch! @SteveSandersonMS - Fixed.

SteveSandersonMS · 2020-06-15T11:11:01Z

src/JSInterop/Microsoft.JSInterop/src/JSRuntime.cs

+        /// <param name="args">JSON-serializable arguments.</param>
+        public async ValueTask InvokeVoidAsync(string identifier, CancellationToken cancellationToken, object[] args)
+        {
+            await this.InvokeAsync<VoidReturn>(identifier, cancellationToken, args);


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

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

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.

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

…e is returned.

campersau · 2020-06-15T16:29:14Z

src/JSInterop/Microsoft.JSInterop/src/JSRuntimeExtensions.cs

@@ -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);


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.

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?

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?

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?

SteveSandersonMS · 2020-07-09T18:24:38Z

src/JSInterop/Microsoft.JSInterop/src/IJSRuntime.cs

+        /// </param>
+        /// <param name="args">JSON-serializable arguments.</param>
+        /// <returns>A <see cref="ValueTask"/> that represents the asynchronous invocation operation.</returns>
+        ValueTask InvokeVoidAsync(string identifier, CancellationToken cancellationToken, object[] args);


Adding these methods to IJSRuntime would be a breaking change. If at all possible, we'd strive to avoid that.

Does it work to not add these, and instead only change the implementation of JSRuntimeExtensions.InvokeVoidAsync? If you changed the implementation of that extension method to call jsRuntime.InvokeAsync<VoidReturn> internally, then JSRuntime.cs could still be aware of that magic return type and use it as a signal not to deserialize the JSON, and just be hardcoded to return default(VoidReturn) which would be discarded by JSRuntimeExtensions.InvokeVoidAsync.

SteveSandersonMS · 2020-07-09T18:35:36Z

Thinking more deeply about this, it occurs to me that rather than simply disregarding the returned JSON, we should really tell the JS-side code not to send any JSON response. Otherwise if the developer calls some function that returns a non-serializable object, then they will get an error even though they told us they didn't want a return value.

It should be possible to tweak Microsoft.JSInterop.ts's invokeJSFromDotNet/beginInvokeJSFromDotNet functions to take an extra treatAsVoid: boolean parameter and use it as a signal to return null in all cases. You'll still want to use your VoidReturn magic type as a way to signal from JSRuntimeExtensions.InvokeVoidAsync/JSInProcessRuntimeExtensions.InvokeVoid to JSRuntime.cs that we're in this situation.

Are you open to a design change like that?

I think this approach is necessary if the goal is really to fix the original issue #16632. The original issue described an error with serializing the JSON response, not with deserializing it.

…okeAsync<VoidReturn>' for each 'InvokeVoidAsync' extension method.

…' method.

StevenRasmussen · 2020-07-09T22:00:35Z

src/JSInterop/Microsoft.JSInterop/src/JSInProcessRuntime.cs

        /// <returns>A JSON representation of the result.</returns>
-        protected abstract string? InvokeJS(string identifier, string? argsJson);
+        protected abstract string? InvokeJS(string identifier, string? argsJson, bool? treatReturnAsVoid);


@SteveSandersonMS - It's not clear to me where this method is implemented? Not sure what needs to be updated to ensure that this new treatReturnAsVoid parameter gets passed to the JavaScript side of things.

Sorry... found it. I initially was just opening the JSInterop.csproj on it's own.... just found Components.slnf which contains everything... now if I can only get the solution to build :(. Some build tasks appear to be failing: Microsoft.AspNetCore.Razor.Tasks.RazorGenerate... any thoughts? It looks like the some dlls in the artifacts folder are not being created.

StevenRasmussen · 2020-07-09T22:03:50Z

@SteveSandersonMS - Your last comments regarding needing to change this at the JS level make sense. I have updated the PR to hopefully reflect this line of thought. Re: tests - I can take a look at this and let you know if I need some guidance and/or someone more familiar with the project to implement.

StevenRasmussen · 2020-07-10T16:18:42Z

src/Components/WebAssembly/JSInterop/src/WebAssemblyJSRuntime.cs

        {
            var noAsyncHandle = default(long);
-            var result = InternalCalls.InvokeJSMarshalled(out var exception, ref noAsyncHandle, identifier, argsJson);
+            var result = InternalCalls.InvokeJSMarshalled(out var exception, ref noAsyncHandle, identifier, argsJson, treatReturnAsVoid);


It looks like from the comments in the InvokeJSMarshalled method that there's some magic that happens here that passes this new boolean arg to JS? Seems like it should just work? Or do we need to be passing the treatReturnAsVoid parameter as part of the argsJson and somehow extracting it on the JS side?

SteveSandersonMS · 2020-07-16T13:06:47Z

It looks like from the comments in the InvokeJSMarshalled method that there's some magic that happens here that passes this new boolean arg to JS? Seems like it should just work?

I recognize it's quite tricky to work out where all the pieces are here and how they all join together! I also realised when thinking about this more that we'll need to be careful about how this works to avoid breaking back-compatibility for people who already have custom implementations of IJSRuntime or JSRuntime etc.

To provide an example of how this could work, I've done a sketch implementation here: https://github.com/dotnet/aspnetcore/compare/stevesa/jsruntime-void-results?expand=1. My sketch implementation would only work if dotnet/runtime#39435 was merged first because it requires a change to the contract between .NET code and the dotnet.wasm runtime (passing an extra treatAsVoid parameter).

Are you interested in taking this sketch and working it up into a finished implementation? It's very close to done, except:

It doesn't have any tests yet
I haven't even run it to check it actually works!

StevenRasmussen · 2020-07-24T20:29:47Z

TBH - This has moved beyond my initial endeavors for a "quick win" and I don't know if I have enough knowledge of the internal workings to take it across the finish line.

mkArtakMSFT · 2020-07-29T16:48:02Z

Thanks for your contribution, @StevenRasmussen.
We realize this went beyond from where this started from. Given the current state we are in in regards to the 5.0 release we prefer to close this now as the work on our plate is higher priority. We may revisit this in the future.

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

defef94

Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 20, 2020

mkArtakMSFT assigned SteveSandersonMS May 27, 2020

watfordgnf reviewed Jun 8, 2020

View reviewed changes

SteveSandersonMS reviewed Jun 15, 2020

View reviewed changes

Set TaskCompletionSource result to 'null' for instances where no valu…

ca19908

…e is returned.

campersau reviewed Jun 15, 2020

View reviewed changes

mkArtakMSFT added area-blazor Includes: Blazor, Razor Components and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jul 8, 2020

SteveSandersonMS reviewed Jul 9, 2020

View reviewed changes

StevenRasmussen force-pushed the invokeVoidAsync-fix branch from 8767c09 to ca19908 Compare July 9, 2020 21:34

StevenRasmussen added 5 commits July 9, 2020 15:36

Merge branch 'master' into invokeVoidAsync-fix

711851c

Remove 'InvokeVoidAsync' method from JSRuntime and intstead call 'Inv…

0ebab51

…okeAsync<VoidReturn>' for each 'InvokeVoidAsync' extension method.

Update JsInProcessRuntimeExtensions to use VoidReturn for 'InvokeVoid…

68c41ab

…' method.

Add new 'treatReturnAsVoid' parameter to 'invokeJSFromDotNet' method.

2691bf1

Added 'treatReturnAsVoid' to InvokeJS method.

8f8d165

StevenRasmussen commented Jul 9, 2020

View reviewed changes

Updated methods to pass allong new 'treatReturnAsVoid' parameter.

e7e0cdb

StevenRasmussen requested a review from a team as a code owner July 10, 2020 16:05

StevenRasmussen commented Jul 10, 2020

View reviewed changes

mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2020

mkArtakMSFT closed this Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

StevenRasmussen commented May 20, 2020

watfordgnf Jun 8, 2020

SteveSandersonMS Jun 15, 2020

StevenRasmussen Jun 15, 2020

SteveSandersonMS Jun 15, 2020

StevenRasmussen Jun 15, 2020

StevenRasmussen Jun 15, 2020

SteveSandersonMS Jun 15, 2020

campersau Jun 15, 2020

StevenRasmussen Jun 15, 2020

StevenRasmussen Jul 9, 2020

SteveSandersonMS Jul 9, 2020

SteveSandersonMS Jul 9, 2020

SteveSandersonMS commented Jul 9, 2020 •

edited

Loading

StevenRasmussen Jul 9, 2020

StevenRasmussen Jul 10, 2020

StevenRasmussen commented Jul 9, 2020

StevenRasmussen Jul 10, 2020

SteveSandersonMS commented Jul 16, 2020

StevenRasmussen commented Jul 24, 2020

mkArtakMSFT commented Jul 29, 2020

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

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

Conversation

StevenRasmussen commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenRasmussen commented Jul 9, 2020

Choose a reason for hiding this comment

SteveSandersonMS commented Jul 16, 2020

StevenRasmussen commented Jul 24, 2020

mkArtakMSFT commented Jul 29, 2020

SteveSandersonMS commented Jul 9, 2020 •

edited

Loading