Fix DispatcherHelper open issues #618

lukasf · 2016-11-13T20:48:30Z

This addresses two issues mentioned in #617.

Use fail fast strategy if the Func or Func<Task> returns null instead of a task.
Also check for null in other occurances of the same problem.

skendrot · 2016-11-14T04:15:01Z

Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs

+                    {
+                        throw new InvalidOperationException("The Task returned by function cannot be null.");
+                    }
+                    await task.ConfigureAwait(false);


Why await the result? Just return the result of the method and remove the async keyword to the anonymous method.

We have to await it because the signature of the lambda is different.

function is type Func<Task> and the lambda using in that call is Func<Task<object>>

By the way, this call with lambda creation was found to be redundant and was removed in my other commit. @lukasf didn't know and went ahead to fix the other error that we found.

Fixed based on code from @Code-ScottLe

skendrot · 2016-11-14T04:15:10Z

Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs

+                    {
+                        throw new InvalidOperationException("The Task returned by function cannot be null.");
+                    }
+                    await task.ConfigureAwait(false);


Fixed based on code from @Code-ScottLe

Code-ScottLe · 2016-11-14T05:04:04Z

@lukasf Just in case you missed my other comment in the issue post, I did fix the problem you pointed out in #617 but haven't yet to do a PR (it was quite late last night). But you already got it going, so let me know. I don't want to be rude and make another PR without you having a say :)

…), fix and extend the docs.

lukasf · 2016-11-14T13:55:33Z

I have updated the PR with a bunch of improvements, largely based on improvements from @Code-ScottLe (thanks!), plus a few more improvements from me. Now I think we have minimized the number of awaits and lambdas to only the bare minimum. This also solves the remarks of @skendrot. Additionally, I improved the docs a bit.

There have been some discussions on the naming in #617. I am not sure on that. After thinking about it, the only name I would like to change would be ExecuteOnUIThreadAsync -> RunOnUIThreadAsync. All other methods start with "Run...", so why do we use "Execute" here? Should we perhaps do this change? Any other ideas on the naming? This is the last chance for renames, once 1.2 is out we should not touch the names anymore.

Misread

Code-ScottLe · 2016-11-14T16:37:21Z

Oh, my initial thought behind that was the inability to have the same name ExecuteOnUIThreadAsync as an overload for CoreApplicationView because we already have an overload in the helper for other purposes. I'm fine with "execute" to "run" as it makes sense.

Code-ScottLe

Some comment regarding improvement of reducing lambda creations

Code-ScottLe · 2016-11-14T16:51:20Z

Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs

@@ -204,17 +188,35 @@ public static Task<T> AwaitableRunAsync<T>(this CoreDispatcher dispatcher, Func<
        /// <returns>Awaitable Task</returns>
        public static Task AwaitableRunAsync(this CoreDispatcher dispatcher, Func<Task> function, CoreDispatcherPriority priority = CoreDispatcherPriority.Normal)
        {
-            return dispatcher.AwaitableRunAsync<object>(


Okay, so i understand that you decide to make each AwaitableRunAsync to deal with the type of function in their own way without relying on the Func<Task<T>> and Func<T> overload to minimize the amount of lambda we have to create. But then again, these are overlapping code and basically we are maintaining 4 different copies of the relatively same piece of code. I don't think having them separated is a good idea imo.

In this case, we save one lambda, one method call and one await. Lambda and method call might not be that much, but await is always pretty expensive (context switch). I do not expect any bigger changes in this area, it's not like this is a user control where new functionality is added into existing code regularly. So I thought it is a one-time-write and then save on every method call. I can revert to the old implementation if you want, but this one is more efficient.

@skendrot complained about unneccessary awaits in the older PR, and I think he is right. Here we can save one more await...

We definitely need more votes on this one. I'm like at the between @deltakosh @ScottIsAFool

This is something we can cleanup after 1.2

Code-ScottLe · 2016-11-14T16:52:52Z

Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs

+                throw new ArgumentNullException("function can't be null!");
+            }
+
+            var taskCompletionSource = new TaskCompletionSource<object>();


Same here. Having to deal with the same code that Func<T> has already had. More relatively-same code to maintain imo.

Agree with Scott
Can we fix it today? I would like to merge it for 1.2

Here we only save one lambda and one method call. No await is saved, so I can revert to the old implementation if you like it better. It won't be much of a difference performance wise.

deltakosh · 2016-11-14T19:25:07Z

Almost good to merge. Just a few comments left

Code-ScottLe · 2016-11-14T19:27:15Z

I actually have a branch with all of the improvement that @lukasf has here and without the overlapping code (we both fixed the issue at the same time but I didn't make the PR fast enough). If we are in a hurry, I can make another PR for my branch and hope that @lukasf won't get mad at me :P

deltakosh · 2016-11-14T19:34:08Z

I'm fine with that:)

Code-ScottLe · 2016-11-14T19:47:15Z

Alright, i will make another PR , there will be a few more things to discuss but i will be available to fix it.

Code-ScottLe · 2016-11-14T19:54:52Z

new PR is #621

lukasf · 2016-11-14T20:01:16Z

Oh well, downsides of working disconnected, two people working in parallel on the same thing... :)

Code-ScottLe · 2016-11-14T20:02:22Z

thread.join();
;)

lukasf · 2016-11-14T20:07:32Z

I fixed the second remark. If I also revert the first remark, it would add one more unneccessary await. Since this is threading stuff and might be called frequently, I think it is a good optimization.

I don't mind if we take @Code-ScottLe's PR or mine. I just want the result to be good.

lukasf · 2016-11-14T20:12:07Z

Oh only when looking at your PR, I saw that the extension methods for CoreApplicationView are not included in the current code. I thought that this was already included. I updated my PR to show you how I imagined it. It does not require new methods, just a slight change to the current ones.

deltakosh · 2016-11-14T22:49:01Z

Waiting for a 2nd approver and I can merge

deltakosh · 2016-11-15T00:10:42Z

Ok I need to merge to be ready for 1.2. Please open an issue if you find something wrong

Fix DispatcherHelper open issues CommunityToolkit#617.

0e172b3

dnfclas added the cla-already-signed label Nov 13, 2016

One more null check.

1c08609

skendrot previously requested changes Nov 14, 2016

View reviewed changes

Further improve DispatcherHelper (reduce number of awaits and lambdas…

1dd65b2

…), fix and extend the docs.

Code-ScottLe suggested changes Nov 14, 2016

View reviewed changes

Revert to old implementation

c0a9fc3

Fix paste error

cde1fee

Add extension method for CoreApplicationView

c576b39

lukasf mentioned this pull request Nov 14, 2016

Dispatcher helper improvement #621

Closed

deltakosh approved these changes Nov 14, 2016

View reviewed changes

deltakosh merged commit 5b153af into CommunityToolkit:dev Nov 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DispatcherHelper open issues #618

Fix DispatcherHelper open issues #618

lukasf commented Nov 13, 2016

skendrot Nov 14, 2016

Code-ScottLe Nov 14, 2016

lukasf Nov 14, 2016

skendrot Nov 14, 2016

lukasf Nov 14, 2016

Code-ScottLe commented Nov 14, 2016 •

edited

lukasf commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

Code-ScottLe left a comment

Code-ScottLe Nov 14, 2016

lukasf Nov 14, 2016

lukasf Nov 14, 2016

Code-ScottLe Nov 14, 2016

deltakosh Nov 14, 2016

Code-ScottLe Nov 14, 2016

deltakosh Nov 14, 2016

lukasf Nov 14, 2016

lukasf Nov 14, 2016

deltakosh commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

deltakosh commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

lukasf commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

lukasf commented Nov 14, 2016

lukasf commented Nov 14, 2016

deltakosh commented Nov 14, 2016

deltakosh commented Nov 15, 2016

Fix DispatcherHelper open issues #618

Fix DispatcherHelper open issues #618

Conversation

lukasf commented Nov 13, 2016

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

Code-ScottLe commented Nov 14, 2016 • edited

lukasf commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

Code-ScottLe left a 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

deltakosh commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

deltakosh commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

lukasf commented Nov 14, 2016

Code-ScottLe commented Nov 14, 2016

lukasf commented Nov 14, 2016

lukasf commented Nov 14, 2016

deltakosh commented Nov 14, 2016

deltakosh commented Nov 15, 2016

Code-ScottLe commented Nov 14, 2016 •

edited