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.

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

Run on same thread in generic host #195

Closed
vlm--- opened this issue Dec 16, 2018 · 4 comments · Fixed by #200
Closed

Run on same thread in generic host #195

vlm--- opened this issue Dec 16, 2018 · 4 comments · Fixed by #200
Assignees
Labels
Milestone

Comments

@vlm---
Copy link
Contributor

vlm--- commented Dec 16, 2018

I was playing with new version and run into some trouble because my code wasn't executed on UI thread. It lead to code in https://github.com/natemcmaster/CommandLineUtils/blob/master/src/Hosting.CommandLine/Internal/CommandLineService.cs class and this method:

public Task<int> RunAsync(CancellationToken cancellationToken)
{
     _logger.LogDebug("Running");
     return Task.Run(() => _state.ExitCode = _application.Execute(_state.Arguments), cancellationToken);
}

It uses Task.Run to execute application login on different thread which is not desired in certain scenarios where you want your code to execute on the UI thread.
It can be easily rewritten to:

public Task<int> RunAsync(CancellationToken cancellationToken)
{
     _logger.LogDebug("Running");
     _state.ExitCode = _application.Execute(_state.Arguments);
     return Task.FromResult(_state.ExitCode);
}

That way the application logic is always executed on same thread. Would you be interested in PR for this change?

@natemcmaster natemcmaster added bug help wanted We would be willing to take a well-written PR to help fix this. labels Dec 17, 2018
@natemcmaster natemcmaster added this to the 2.3.0 milestone Dec 17, 2018
@natemcmaster
Copy link
Owner

Sure, I'd take a PR. @lucastheisen would this change be fine?

@lucastheisen
Copy link
Contributor

No objection here.

Honestly, I don't have any UI use cases, so have no idea what the implications would be there. I don't imagine it would hurt anything.

@TheConstructor
Copy link
Contributor

@vlm would this also work? #196

@vlm---
Copy link
Contributor Author

vlm--- commented Dec 17, 2018

It should work and would be better because async should be all the way from top to bottom and Task.FromResult is just like any other task (some results are internally cached), see first chapter in https://blogs.msdn.microsoft.com/dotnet/2018/11/07/understanding-the-whys-whats-and-whens-of-valuetask/ for better explanation. No need for my PR anymore.

natemcmaster added a commit that referenced this issue Dec 19, 2018
@natemcmaster natemcmaster self-assigned this Dec 19, 2018
@natemcmaster natemcmaster removed the help wanted We would be willing to take a well-written PR to help fix this. label Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants