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

[ReactiveUI 6.4.x] AsyncTask not running on background thread in WPF application #813

Closed
xdarsie opened this issue Feb 25, 2015 · 6 comments
Labels

Comments

@xdarsie
Copy link

xdarsie commented Feb 25, 2015

Hello,

I am not sure if this is a bug in my code, a rxui bug or if I am just misunderstanding the expected behavior of the framework.

I have pretty much the same problem/issue posted on this SO question. Basically, I am not able to automagically run IO operations in a background thread. I am overcoming this by manually firing off new tasks for each IO operation, but I would like to stop doing that as (if I understood correctly) it "should not" be necessary.

I have pasted some code that I am using to track threads.

C# code

using System;
using System.Net.Http;
using System.Reactive.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using ReactiveUI;

namespace WpfApplication1
{
    public class MainWindowsViewModel : ReactiveObject
    {
        private string m_log;
        private string m_output;

        private readonly StringBuilder m_stringBuilder;

        public ReactiveCommand<string> FetchData { get; set; }

        public string Log
        {
            get { return m_log; }
            set { this.RaiseAndSetIfChanged(ref m_log, value); }
        }

        public string Output
        {
            get { return m_output; }
            set { this.RaiseAndSetIfChanged(ref m_output, value); }
        }

        public MainWindowsViewModel()
        {
            m_stringBuilder = new StringBuilder();

            m_stringBuilder.AppendFormat("Ctor: Thread {0} {1}{2}",
                Thread.CurrentThread.ManagedThreadId,
                Thread.CurrentThread.IsBackground ? "Background" : "Dispatcher",
                Environment.NewLine);

            var dataSource = new DataSource(m_stringBuilder);

            FetchData = ReactiveCommand.CreateAsyncTask(async _ => await dataSource.GetData());

            FetchData.
                SubscribeOn(RxApp.TaskpoolScheduler).
                ObserveOn(RxApp.MainThreadScheduler).
                Subscribe(SetProperties);

            FetchData.ExecuteAsyncTask();
        }

        private void SetProperties(string data)
        {
            m_stringBuilder.AppendFormat("SetProperties: Thread {0} {1}{2}",
                Thread.CurrentThread.ManagedThreadId,
                Thread.CurrentThread.IsBackground ? "Background" : "Dispatcher",
                Environment.NewLine);

            Output = data;
            Log = m_stringBuilder.ToString();
        }
    }

    public class DataSource
    {
        private readonly StringBuilder m_builder;

        public DataSource(StringBuilder builder)
        {
            m_builder = builder;
        }

        public async Task<string> GetData()
        {
            // PROBLEM HERE: I am expecting Thread.CurrentThread.IsBackground to be true
            // Firing a new Task solves the problem

            //var task = Task.Factory.StartNew(async () =>
            //{
            m_builder.AppendFormat("GetData: Thread {0} {1}{2}",
                Thread.CurrentThread.ManagedThreadId,
                Thread.CurrentThread.IsBackground ? "Background" : "Dispatcher",
                Environment.NewLine);

            using (var client = new HttpClient())
            {
                using (var response = await client.GetAsync(new Uri("http://www.google.com")))
                {
                    using (var content = response.Content)
                    {
                        var html = await content.ReadAsStringAsync();

                        return string.Format("Reloaded {0} times - {1}", 
                            s_count++, html.Remove(s_count * 2));
                    }
                }
            }
            //});

            //return await task.Result;
        }

        private static int s_count;
    }
}

XMAL code

<Window x:Class="WpfApplication1.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Title="MainWindow" Height="350" Width="525">
    <StackPanel>
        <TextBlock Text="{Binding Output}"></TextBlock>
        <Button Command="{Binding Path=FetchData}">Reload</Button>
        <TextBlock Text="{Binding Log}"></TextBlock>
    </StackPanel>
</Window>

According to Paul's answer, "ReactiveCommand should guarantee that even async operations that accidentally burn CPU before running an async op are forced onto background threads". However, that's not what I am seeing. My app runs always on the UI thread.

blah

Now the questions. Is this the expected behavior? Is my sample code wrong? Is there a known issue in the rxui code base?

Thanks

@rikbosch
Copy link
Contributor

The SynchronizationContext is probably captured when you created the ReactiveCommand using async / await syntax. This will result in the continuations (lines after the awaits) being run on the dispatcher thread. Please note that the actual work ( client.GetAsync and ReadAsStringAsync()) are executed on a threadpool thread

try to use the Task directly:

 FetchData = ReactiveCommand.CreateAsyncTask(_=>dataSource.GetData());

edit:

furthermore:

The SubscribeOn call can be omitted. Subscribing to an ReactiveCommand, will run on the RxApp.MainThreadScheduler by default.

I think the SubscribeOn can also be omitted. @paulcbetts please correct me if I'm wrong.

   FetchData.Subscribe(SetProperties);

@xdarsie
Copy link
Author

xdarsie commented Feb 25, 2015

Thanks Rik for you answer.

I should have mentioned that I tried using the task directly and I got the same result. The SO answer also mentioned that SubscribeOn & ObserveOn are not required, but having them or not does not change the end result.

Thanks for pointing out that the async methods inside GetData() are executed on a bg thread, but I would like to execute GetData in a bg thread as well. This is just a simple example.

@anaisbetts
Copy link
Member

I think you may be misreading my SO post, which actually says the opposite, that RxUI 6.x does not guarantee it will be run on another thread, if the async method below it is badly written. In this case, because HttpClient is doing blocking operations when it shouldn't, you need to wrap it in a Task.Start

@xdarsie
Copy link
Author

xdarsie commented Feb 25, 2015

Thanks for you answer. I see now what you meant on your SO answer, sorry for that.

Going back to the latest reply: What do yo mean by "if the async method below it is badly written"? I do not quite get why this is the case. I am sorry if I am missing something obvious.

If my GetData method (which is just an example, not my real code) does a lot of heavy computations and/or many IO operations, it would be great to tell RxUI to execute it in a new task. How can I achieve that without Task.Start? Is it possible? How can I tweak this GetData example so it is no "badly written" anymore?

Thanks

@anaisbetts
Copy link
Member

Going back to the latest reply: What do yo mean by "if the async method below it is badly written"?

So, HttpClient as I recall, does something kind of dumb - while it guarantees that the actual request is done on another thread, the DNS lookup often ends up being on the UI thread, before it calls an async method, so you lock up.

In general, async is just a handshake between two methods, "I promise not to block the UI thread", but that promise doesn't have teeth - badly written methods can totally block the calling thread.

If my GetData method (which is just an example, not my real code) does a lot of heavy computations and/or many IO operations, it would be great to tell RxUI to execute it in a new task. How can I achieve that without Task.Start?

Using Task.Start is an entirely reasonable way to guarantee that code run on the background thread

@xdarsie
Copy link
Author

xdarsie commented Feb 26, 2015

Ok, got it now. I thought I was missing something different. I was under the wrong impression that RxUI was in charge of executing in the BG those commands that were created using CreateAsyncTask (which, BTW, would be a nice feature).

Thanks for your answers.

@lock lock bot added the outdated label Jun 26, 2019
@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants