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

unblock main thread when fetching user votes #14793

Merged
merged 5 commits into from
Feb 12, 2024
Merged

unblock main thread when fetching user votes #14793

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a286988
unblock main thread when fetching user votes
dnenov Dec 20, 2023
b65ffd5
Merge remote-tracking branch 'upstream/master' into pm-uservotes-unbl…
dnenov Jan 12, 2024
e822933
moved uservotes call closer to execution
dnenov Jan 12, 2024
9598a4f
Merge branch 'master' into pr/14793
pinzart90 Feb 7, 2024
63d880a
Update PackageManagerClientViewModel.cs
pinzart90 Feb 7, 2024
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
4 changes: 2 additions & 2 deletions src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public string Username
public ICommand ToggleLoginStateCommand { get; private set; }

/// <summary>
/// Contains all votes the user has been submitted.
/// Contains all votes the user has submitted.
/// Will allow the user to vote for a package they have not upvoted before
/// </summary>
private List<string> Uservotes { get; set; }
Expand All @@ -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());
Copy link
Member

@mjkkirschner mjkkirschner Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

image

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@dnenov dnenov Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
}

Expand Down