unblock main thread when fetching user votes #14793

dnenov · 2023-12-20T18:10:42Z

Purpose

A small PR introducing an async loading of UserVotes to unload the main thread from potentially long operation (under slow or now internet?) and unblock Dynamo loading process.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

before fetching user votes could potentially slow down dynamo loading as it was being executed on the main thread
now using Task.Run to execute on a separate thread and unblock the loading process

Reviewers

@mjkkirschner
@aparajit-pratap

FYIs

@pinzart

- before fetching user votes could potentially slow down dynamo loading as it was being executed on the main thread - now using Task.Run to execute on a separate thread and unblock the loading process

mjkkirschner · 2023-12-21T20:01:57Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -285,7 +285,7 @@ internal PackageManagerClientViewModel(DynamoViewModel dynamoViewModel, PackageM

            if (AuthenticationManager.LoginState.Equals(LoginState.LoggedIn))
            {
-                this.Uservotes = this.Model.UserVotes();
+                Task.Run(() => this.Uservotes = this.Model.UserVotes());                


this is going to execute from other some other thread pool thread.

Does anything else write to UserVotes? (another thread?) - it seems like it could be made readonly if not.

Hi, sorry for the late reply on this thread!

I am not sure if I got this correctly - the UserVotes() returns the results of the Greg call:

It is only used once when we create the PackageManagerClientViewModel and fetches any votes from the current user, I think that's about it.

I say keep it in a Task.Run, but move it closer to the greg client call.
Having it in the VIewModel is risky, because someone might add UI code (code that interacts with UI elements) in the UserVotes body
As @mjkkirschner mentioned, we need to make sure all data that is accessed by other thread is thread safe (in some way or another)
ALso we need to make sure we handle exceptions properly. If ExecuteAndDeserialize throws an exception, what happens?

I think I got it. I moved the call to right before using the Uservotes for the first time.

mjkkirschner · 2023-12-21T20:41:09Z

hey @pinzart90 what do you think about this note a the very start of the docs...

https://learn.microsoft.com/en-us/dotnet/csharp/asynchronous-programming/async-scenarios#overview-of-the-asynchronous-model

For I/O-bound code, you await an operation that returns a Task or Task<T> inside of an async method.
For CPU-bound code, you await an operation that is started on a background thread with the Task.Run method.

If the work you have is I/O-bound, use async and await without Task.Run. You should not use the Task Parallel Library.

I think in general refactoring all of our code to be async/await might not be worth the pain - though I guess the point of the above advice is that eventually if we continue this pattern of moving io bound code to threads, we'll be using threads just to unblock the main thread and taking on that overhead when we could have used async/await and avoided the thread overhead/ potential deadlocks,livelocks.

pinzart90 · 2023-12-21T21:00:13Z

hey @pinzart90 what do you think about this note a the very start of the docs...

https://learn.microsoft.com/en-us/dotnet/csharp/asynchronous-programming/async-scenarios#overview-of-the-asynchronous-model

For I/O-bound code, you await an operation that returns a Task or Task<T> inside of an async method. For CPU-bound code, you await an operation that is started on a background thread with the Task.Run method.

If the work you have is I/O-bound, use async and await without Task.Run. You should not use the Task Parallel Library.

I think in general refactoring all of our code to be async/await might not be worth the pain - though I guess the point of the above advice is that eventually if we continue this pattern of moving io bound code to threads, we'll be using threads just to unblock the main thread and taking on that overhead when we could have used async/await and avoided the thread overhead/ potential deadlocks,livelocks.

I know we talked about this in the past a bit:
https://autodesk.slack.com/archives/C01C8GGT9EY/p1701112257601019?thread_ts=1701109330.370119&cid=C01C8GGT9EY

My take on it is that we should not block use/block threadPool threads for something as simple as I/O code (file read/write, web access etc) when we already have an awaitable I/O operation to deal with. It might not be simple to refactor existing code to go from Task.Run to async/await. Example : all of our Greg code (that deals with packageManager requests) is not awaitable, so of course it is easier to stick it in another thread.
I say let's not refactor existing code just yet, but keep this in mind going forward.

…ocking-main-thread

reddyashish · 2024-01-12T18:58:17Z

@mjkkirschner @pinzart Just to confirm, do we want to hold off this change for 3.0.2 and refactor it in 3.1?

github-actions · 2024-01-12T19:07:57Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

github-actions · 2024-01-12T19:57:23Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

pinzart90 · 2024-01-16T16:09:39Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -482,6 +477,12 @@ public List<PackageManagerSearchElement> ListAll()
        {
            CachedPackageList = new List<PackageManagerSearchElement>();

+            // Attempt to load user votes prior to using it
+            if (AuthenticationManager.LoginState.Equals(LoginState.LoggedIn) && Uservotes == null)


So you are basically caching UserVotes.
Will it be a problem that UserVotes will not refresh for subsequent calls to public List<PackageManagerSearchElement> ListAll() ?

There is a 15 minute lag until the database picks up the changes, as far as I know. The data is specific to the user. Which means, that they won't be able to see the subsequent change until the database picks it up, again this is as far as my knowledge goes on how fast the data will be registered.

With that, I think there is no problem that the UserVotes won't refresh in subsequent calls of ListAll().

pinzart90 · 2024-01-16T16:24:04Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

+            // Attempt to load user votes prior to using it
+            if (AuthenticationManager.LoginState.Equals(LoginState.LoggedIn) && Uservotes == null)
+            {
+                Task.Run(() => this.Uservotes = this.Model.UserVotes());


also Task.Run() will run asynchronously on another thread. That means that the rest of the code will execute without waiting for the result.
Also won't Model.ListAll() cause the same UI blocking issue ?

Ideally we should make all these methods async and cascade them down to a UI method that makes sense to be async. Something like DynamoViewModelRequestShowPackageManagerSearch should be async (and all the async-able calls downstream) and the UI should relate that it is doing something async (UI spinner etc). I realize this is too much for 3.0.x release.

I am no longer sure how we can make this work properly, since a part of the code needs the information synchronously (which means the UI thread needs to wait for that information)

OF course, just by moving the offending outside the constructor, you will no longer block the DynamoView initialization code (basically Dynamo load logic). We will now block the UI, but later on when the user actually requests PackageManager information.
Maybe we it is better than nothing.

We could also try to parallelize the calls something like

IEnumerable<PackageHeader> allPackages; var task1 = Task.Run(() => Model.UserVotes()); var task2 = Task.Run(() => Model.ListAll()); Task.WhenAll(task1, task2).Wait(); Uservotes = task1.Result; allPackages = task2.Result;

Sorry, looks like there are some unresolved discussions around REstSHarp's thread safety.
I think it is ok to call it from multiple threads, just not in parallel
So, runnig the code on the UI thread might be safer (There is no real value in using Task.Run except if you can do it async or parallelize it)

dnenov · 2024-01-26T17:59:46Z

Hi @pinzart90 @mjkkirschner - would it be OK to merge this PR at this point? Or close it down in favor of a more centralized effort to align the correct behavior of all Greg async calls? Just to remind us, this PR addresses a lag/freeze on Dynamo loading on slow machines blocking the main thread detected by @mjkkirschner. It is probably a very minor change relative to the holistic conversation about thread safety.

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

pinzart90 · 2024-02-07T14:38:43Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -482,6 +477,8 @@ public List<PackageManagerSearchElement> ListAll()
        {
            CachedPackageList = new List<PackageManagerSearchElement>();

+            // Calls to Model.UserVotes and Model.ListAll might take a long time to run (so do not use them syncronously in the UI thread)


public List<PackageManagerSearchElement> ListAll() is called from an async method upstream
It is not going to block the UI thread anymore.

We do need a clear way to mark these methods (that they are Task based or async) . This will be part of another task

what async method calls ListAll()?

Dynamo/src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerSearchViewModel.cs

Line 1026 in d01e057

Task<IEnumerable<PackageManagerSearchElementViewModel>>.Factory.StartNew(RefreshAndSearch).ContinueWith((t) =>

unblock main thread when fetching user votes

a286988

- before fetching user votes could potentially slow down dynamo loading as it was being executed on the main thread - now using Task.Run to execute on a separate thread and unblock the loading process

aparajit-pratap approved these changes Dec 21, 2023

View reviewed changes

mjkkirschner reviewed Dec 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pm-uservotes-unbl…

b65ffd5

…ocking-main-thread

moved uservotes call closer to execution

e822933

pinzart90 reviewed Jan 16, 2024

View reviewed changes

pinzart90 reviewed Jan 26, 2024

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Show resolved Hide resolved

pinzart90 added 2 commits February 7, 2024 09:30

Merge branch 'master' into pr/14793

9598a4f

Update PackageManagerClientViewModel.cs

63d880a

pinzart90 reviewed Feb 7, 2024

View reviewed changes

aparajit-pratap approved these changes Feb 7, 2024

View reviewed changes

pinzart90 merged commit 0435f08 into DynamoDS:master Feb 12, 2024
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unblock main thread when fetching user votes #14793

unblock main thread when fetching user votes #14793

dnenov commented Dec 20, 2023 •

edited

Loading

mjkkirschner Dec 21, 2023 •

edited

Loading

dnenov Jan 12, 2024

pinzart90 Jan 12, 2024

dnenov Jan 12, 2024 •

edited

Loading

mjkkirschner commented Dec 21, 2023 •

edited

Loading

pinzart90 commented Dec 21, 2023 •

edited

Loading

reddyashish commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

pinzart90 Jan 16, 2024

dnenov Jan 26, 2024

pinzart90 Jan 16, 2024

pinzart90 Jan 16, 2024

pinzart90 Jan 16, 2024

dnenov commented Jan 26, 2024

pinzart90 Feb 7, 2024 •

edited

Loading

mjkkirschner Feb 7, 2024

pinzart90 Feb 7, 2024

unblock main thread when fetching user votes #14793

unblock main thread when fetching user votes #14793

Conversation

dnenov commented Dec 20, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

mjkkirschner Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnenov Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Dec 21, 2023 • edited Loading

pinzart90 commented Dec 21, 2023 • edited Loading

reddyashish commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

UI Smoke Tests

github-actions bot commented Jan 12, 2024

UI Smoke Tests

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

dnenov commented Jan 26, 2024

pinzart90 Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnenov commented Dec 20, 2023 •

edited

Loading

mjkkirschner Dec 21, 2023 •

edited

Loading

dnenov Jan 12, 2024 •

edited

Loading

mjkkirschner commented Dec 21, 2023 •

edited

Loading

pinzart90 commented Dec 21, 2023 •

edited

Loading

pinzart90 Feb 7, 2024 •

edited

Loading