0

(Problem solved) I have an MVC application, in my Action:

First case: Task never started.

public ActionResult Insert(NewsManagementModel model) {
            //Do some stuff

            //Insert history  
            //new object NewsHistoryDto  as the parameter
            Task.Factory.StartNew(() => InsertNewsHistory(new NewsHistoryDto {
                UserId = 1234,
                Time = DateTime.Now,
                Id = 1234
            })); 
            return RedirectToAction("Index", "NewsManagement");
        }

Second case: Task run normally

public ActionResult Insert(NewsManagementModel model) {
            //Do some stuff

            //Insert history 
            //object NewsHistoryDto was declared outside
            var history = new NewsHistoryDto {
                UserId = 1234,
                Time = DateTime.Now,
                Id = 1234
            }; 
            Task.Factory.StartNew(() => InsertNewsHistory(history)); 
            return RedirectToAction("Index", "NewsManagement");
        }

My question is: When Task.Factory.StartNew and i put a method into it, the parameter of that method (object) must be declare outside??? Because when i write shortly like the first case, i put the "new" keyword in the parameter and the task never run. Reason: In action, i wanna return view as soon as possible, any other stuff not related to that view will be excute in a task and client no need to wait for completed.

I'm very sorry about my bad english :)

Updated 1: Thanks Panagiotis Kanavos, I used QueueBackgroundWorkItem but the problem still the same, if i declare the object outside, this method run normally. But when i use new keyword inside the parameter, this method never run. No exception, no errors. Can anyone explain to me how this possible :(

Updated 2: I try two case:

First:

    HostingEnvironment.QueueBackgroundWorkItem(delegate {
        var handler = m_bussinessHandler;
        handler.InsertNewsHistoryAsync(new NewsHistoryDto {
            UserId = UserModel.Current.UserId,
            Time = DateTime.Now,
            Id = newsId
        });
    });-> still doens't works

Second:

        var history = new NewsHistoryDto {
            UserId = UserModel.Current.UserId,
            Time = DateTime.Now,
            Id = newsId
        };

        HostingEnvironment.QueueBackgroundWorkItem(delegate {
            var handler = m_bussinessHandler;
            handler.InsertNewsHistoryAsync(history);
        });-> works normally

So where is the problem here??? That's not about m_bussinessHandler because i copied.

Updated 3: I found the reason. The reason is UserModel.Current, this is an object in HttpContext.Current.Session["UserModel"], in this case when i call async method, when this method actually excute, it can access to HttpContext.Current which is null. So i can solve this problem by declare object outside to store data and pass it into method or I capture UserModel.Current and pass it into this method to use UserModel.Current.UserId.

My problem actually solved, thanks everyone for helping me, especially Panagiotis Kanavos.

Community
  • 1
  • 1
toannm
  • 485
  • 4
  • 9
  • How do you know it wasn't started when you never check the task's status? The operation may have been aborted because of the way the entire action is written or an exception may have occurred. Did you really want to return to the client before inserting the record in the database? – Panagiotis Kanavos Mar 10 '15 at 08:24
  • I debuged and no exception. The second case when i debug, it run into the method InsertNewsHistory normally. The first case when i debug, it never run into the method InsertNewsHistory. Because this method must insert/update the history of thousand records, and the client no need to wait for it. It just for logging. – toannm Mar 10 '15 at 08:26
  • First, orphan tasks can be aborted when an HTTP request finishes. Your code is unsafe. Second, unless you've put a breakpoint inside InsertNewsHistory, you won't be able to step into a task before the action finishes - that's what asynchronous means. Finally, the debugger freezes threads and executes only one step at a time - your task may not have had a chance to start before the action finished. – Panagiotis Kanavos Mar 10 '15 at 08:29
  • Yes, when i debuged, I put a breakpoint inside InsertNewsHistory and the second case it excute normally and the first case it never excute. I dont know the different between two cases :( – toannm Mar 10 '15 at 08:34
  • Only chance. The code is *unsafe*, you are creating an orphaned task that *can* be killed by IIS. Or it may be causing exceptions that you'll never notice. Use proper `async/await` methods instead. – Panagiotis Kanavos Mar 10 '15 at 08:35
  • Can you give me some advise for this problem? I wanna do InsertNewsHistory but client no need to wait for it because this method take about 10 second, it took many time and client no need to wait for it. – toannm Mar 10 '15 at 08:38

2 Answers2

0

Is your m_bussinessHandler an instance field? Because it can got disposed after you finished the actions.

João Silva
  • 569
  • 3
  • 9
  • m_bussinessHandler just a class for handler bussiness, the important is i pass a method into Task.Factory.Startnew. In both case, the different is the parameter inside the method.1 new, 1 declare outside. When "new", it never excute, but when declare outside, it excute normally. – toannm Mar 10 '15 at 08:19
  • If you put m_businessHandler static you won't have that problem anymore. I'll tell you why in a couple of hours – João Silva Mar 10 '15 at 08:37
  • Using static fields is always a bad idea - it prevents garbage collection, can lead to resource leaks if any connections inside the static aren't cleared after every call and makes testing a lot harder. Disposal is a *good thing*, not a bug to overcome, especially in server applications – Panagiotis Kanavos Mar 10 '15 at 08:44
  • The problem is caused because the code captures `m_businessHandler` by reference, ie it captures its address, not its value. The [solution](http://stackoverflow.com/questions/451779/how-to-tell-a-lambda-function-to-capture-a-copy-instead-of-a-reference-in-c) is to copy the value inside the lambda, not introduce a static. – Panagiotis Kanavos Mar 10 '15 at 10:03
  • I completely agree. I was going to explain what you explained on you answer. The reason for the static was only to prove the point. This code just don't work because `m_bussinessHandler` has already been disposed. – João Silva Mar 10 '15 at 10:27
0

As it is, your code returns to the caller before the task finishes. The task may not have even started executing by the time the code returns, especially if you are debugging the method. The debugger freezes all threads and executes only one of them step by step.

Moreover, .NET's reference capture means that when you use m_businessHandler you capture a reference to the m_businessHandler field, not its value. If your controller gets garbage collected, this will result in a NullReferenceException. To avoid this you have to make a copy of the field's value inside your lambda.

You should write a proper asynchronous method instead, that returns to the user only when the asynchronous operation completes:

public async Task<ActionResult> Insert(NewsManagementModel model) {
    //Do some stuff

    //Insert history  
    //new object NewsHistoryDto  as the parameter
    await Task.Run(() => 
        var handler=m_bussinessHandler;
        handler.InsertNewsHistory(new NewsHistoryDto {
            UserId = 1234,
            Time = DateTime.Now,
            Id = 1234
    })); 
    return RedirectToAction("Index", "NewsManagement");
}

Task.Run or Task.Factory.StartNew are roughly equivalent.

Even so, it's not a good idea to use Task.Run to fake asynchronous execution - you simply switched execution from one server thread to another. You should use asynchronous methods all the way down to the database, eg. use ExecuteNonQueryAsync if you are using ADO.NET or SaveChangesAsync in Entity Framework. This way no thread executes or blocks while the code waits for the database call to complete.

The compiler also takes care of capturing the field's value so you don't need to copy anything. The resulting code is a lot cleaner:

public async Task<ActionResult> Insert(NewsManagementModel model) {
    //Do some stuff

    //Insert history  
    //new object NewsHistoryDto  as the parameter
    await m_bussinessHandler.InsertNewsHistoryAsync(new NewsHistoryDto {
            UserId = 1234,
            Time = DateTime.Now,
            Id = 1234
    }; 
    return RedirectToAction("Index", "NewsManagement");
}

If you do want the operation to run in the background and return to the client immediately, you can use QueueBackgroundWorkItem which starts a new Task and registers it with IIS. In this case you have to copy the field's value again:

public ActionResult Insert(NewsManagementModel model) {
    //Do some stuff

    //Insert history  
    //new object NewsHistoryDto  as the parameter
    HostingEnvironment.QueueBackgroundWorkItem(ct=>
        var handler=m_bussinessHandler;
        handler.InsertNewsHistoryAsync(new NewsHistoryDto {
            UserId = 1234,
            Time = DateTime.Now,
            Id = 1234
    }); 
    return RedirectToAction("Index", "NewsManagement");
}

IIS can still cancel the task, eg when the application pool recycles. Before it does so, it will notify the task through the CancellationTask passed to the lambda (the ct parameter). If the task doesn't finish in time, IIS will go on and abort the thread anyway. A long running task should check the token periodically.

You can pass the cancellation token to most asynchronous methods, eg ExecuteNonQueryAsync(CancellationToken). This will cause the IO operation to cancel as soon as it's safe to do so instead of waiting until the remote server responds.

Scott Hanselman has a nice article describing all the ways available to rung tasks and even scheduled jobs in the background in How to run Background Tasks in ASP.NET

Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • But client must wait for the task completed and i dont want this. Because this method take a long time to completed and client no need to wait. I google and http://brian-federici.com/blog/2013/7/8/ensuring-tasks-complete-in-aspnet-mvc. Can i use this? – toannm Mar 10 '15 at 08:42
  • Scott Hansleman wrote a [blog post](http://www.hanselman.com/blog/HowToRunBackgroundTasksInASPNET.aspx) covering the options. In your case, you may be able to simply use [QueueBackgroundWorkItem](http://blogs.msdn.com/b/webdev/archive/2014/06/04/queuebackgroundworkitem-to-reliably-schedule-and-run-long-background-process-in-asp-net.aspx) which notifies IIS that there is a task running. Updating the answer – Panagiotis Kanavos Mar 10 '15 at 08:48
  • I tried HostingEnvironment.QueueBackgroundWorkItem(ct => m_bussinessHandler.InsertNewsHistoryAsync(newsHistory)); The problem still the same, when i declare the object ouside, it excute normally, but when i use "new" object inside the parameter, it never excute. Maybe this is a bug of .NET ? – toannm Mar 10 '15 at 09:44
  • There are several millions of lines of code that work with `new` inside tasks. It's far more likely that an exception occurs, eg. because you reference a field variable like `m_bussinessHandler` that no longer exists. The way the original code was written, it captures the field's address, not its value. Updated the answer to avoid this. Note that when using `await` the compiler takes care of capturing – Panagiotis Kanavos Mar 10 '15 at 10:01
  • *Did* you add any exception handling to check for errors? The NullReferenceException is the primary reason the original code fails. There may be other issues eg. where did `UserModel` come from? Is it or its `Current` field initialized? – Panagiotis Kanavos Mar 10 '15 at 10:24
  • Yes, no errors :( I really feeling crazy with this problem – toannm Mar 10 '15 at 10:28