2
\$\begingroup\$

I have an ASP.NET MVC 5 application that needs to log to the database every request for a specific controller action and every link click that happens on the view of that action. Since the user shouldn't be affected of this operation.

I came up with the following pattern:

For the controller:

//get the repository instance
private RequestsRepository _repo = new RequestsRepository();
public ActionResult Index()
{
    //log the request in a new Task
    Task.Factory.StartNew(() =>
    {
        _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
    });

    return View();
}

For the view (client side):

<!--each link is decorated with the "trackedlink" class-->
<a href="http://1234.com" class="trackedlink">This is a link</a>
<script>
    //on document ready the link click event is registered
    $(document).ready(function () {
        $('.trackedlink').click(function () {
            //use $.get method to register the click
            //LinkLogger is a public method in the Home Controller
            $.get('@Url.Action("LinkLogger", "Home")', function (data) { });
        });
    });
</script>

Server side method for link click logging:

public void LinkLogger()
{
    Task.Factory.StartNew(() =>
    {
        _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
    });
}

Repository method:

public void RegisterRequest(string ip, string userAgent)
{
    string sql = @"INSERT INTO RequestsLog (Ip, UserAgent) VALUES (@Ip, @UserAgent)";
    using (var conn = ConnectionStringProvider.GetOpenConnection())
    {
        conn.Execute(sql, new { Ip = ip, UserAgent = userAgent);
    }
}

Is this a right way to implement the requests?

\$\endgroup\$
2
\$\begingroup\$

I would say the short answer to your questions is no.

But since this is code review a deeper explanation is required.

Natual vs. Supernatural Async

Logging every request to a database is a common practice but can be done many different ways (IIS does this for you by default BTW). Generally speaking the desire is usually something like this:

"We need to track user metrics or gather data to help identify potentially malicious activity on our web-server. Let's grab all user information (with time stamp) so we can get better visibility into what is happening. In order to pull this off though, we need to not impact normal site operations."

Ajax (to me) is the ideal candidate since it will be made on a separate request and any DB connectivity issues, latency, errors etc will not affect the actual application receiving the request.

The question is what level of detail is needed for logging. Your code seems to indicate that all that is needed is the client IP and User-Agent. That being the case, I would opt for a pure JS solution that makes a naturally asynchronous call and logs that information.

By using Ajax you're working with the expectations of the web server (at least in an HTTP/1 world) which expects to generally use 1 thread per request to retrieve 1 resource.

By using the TaskFactory or any form of the TPL for asynchronous processing, you're potentially breaking the model.

On the surface, this is a technicality, but if we dig deeper, we find that there are some scalability issues here.

Stephen Cleary (and others) write extensively on this subject so I will include some links to additional reading but allow me to summarize here:

Task.Run is simply a wrapper introduced in .NET 4.5 on top of TaskFactory.StartNew, which had a more complicated signature. For more on this, see here: https://blogs.msdn.microsoft.com/pfxteam/2011/10/24/task-run-vs-task-factory-startnew/

The behavior of this method is to create a new thread if no thread is available.

Below is a quote from a Stephen Cleary article that succinctly summarizes what happens when Task.Run fires (particularly in an ASP.NET Environment).

Taken from: http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html

  • The request starts processing on an ASP.NET thread.
  • Task.Run starts a task on the thread pool to do the calculations. The ASP.NET thread pool has to deal with (unexpectedly) losing one of its threads for the duration of this request.
  • The original request thread is returned to the ASP.NET thread pool.
  • When the calculation is complete, that thread completes the request and is returned to the ASP.NET thread pool. The ASP.NET thread pool has to deal with (unexpectedly) getting another thread.

If you have multiple calls to Task.Run, then the performance problems are compounded. On a busy server, this kind of implementation can kill scalability.

That’s why one of the principles of ASP.NET is to avoid using thread pool threads (except for the request thread that ASP.NET gives you, of course). More to the point, this means that ASP.NET applications should avoid Task.Run.

Code

So, in your JS, I would recommend the following approach:

<!--each link is decorated with the "trackedlink" class-->
<a href="http://1234.com" class="trackedlink">This is a link</a>
<script>
    //on document ready the link click event is registered
    $(document).ready(function () {
        $.get('@Url.RouteUrl("HomeLog")', function(data) { }); // Make the call on the index page

        $('.trackedlink').click(function () {
            //use $.get method to register the click
            //LinkLogger is a public method in the Home Controller
            $.get('@Url.Action("LinkLogger", "Home")', function (data) { });
        });
    });
</script>

Then in your server-side code, you can remove those task runs completely (as from JS they don't serve a purpose).

//get the repository instance
private RequestsRepository _repo = new RequestsRepository();

public ActionResult Index()
{    
    return View();
}

public ActionResult HomeLog() {
    _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
}

public void LinkLogger()
{
    _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
}

And if you really wanted to make those ajax calls more "parallel" you can decorate the actions using async / await.

Here are some articles on the subject for further reading:

\$\endgroup\$
  • \$\begingroup\$ This is very interesting, thanks a lot for the very detailed answer. One question in mind is if using new Thread(()=>{}).Start(); will eliminate the problems you mentioned above since it won't have an affect on the thread pool \$\endgroup\$ – Yoav Oct 10 '16 at 19:00
  • \$\begingroup\$ I have run into this scenario numerous times in the past myself (and experienced the aforementioned scale issues). I appreciate how straightforward your approach is. You might also want to get your hands on a copy of amazon.com/Concurrency-C-Cookbook-Stephen-Cleary/dp/1449367569 I know it was a gem for me. \$\endgroup\$ – xDaevax Oct 10 '16 at 19:07
  • \$\begingroup\$ Technically no, using a thread directly would not take a thread away from the threadpool but in so doing, you lose some other benefits. Namely you will lose context information (since the HttpContext.Current is really just accessing ThreadLocal storage). As far as the usage of Thread itself, I tend to avoid it. I can go into more detail if you need, but reading these articles should help illustrate why: blog.slaks.net/2013-10-11/threads-vs-tasks yoda.arachsys.com/csharp/threads/printable.shtml <-- The part about "The Thread Pool and Asynchronous Methods". \$\endgroup\$ – xDaevax Oct 10 '16 at 19:17
  • \$\begingroup\$ What I like most in your review are the links to additional articles ;-) \$\endgroup\$ – t3chb0t Oct 11 '16 at 8:52
  • \$\begingroup\$ @t3chb0t Thanks. I like to have research / authoritative information to back up personal opinion usually. I find it's more compelling if I'm not just making things up. \$\endgroup\$ – xDaevax Oct 12 '16 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.