6
\$\begingroup\$

I've been implementing a custom internal server error page in ASP.Net MVC which will check if the current user is either an administrator or accessing the page from localhost, and if so, show them a whole bunch of details about the error to debug it with, otherwise just send them to a basic HTML error page.

So far, it works great, but one problem I had was that if there is an error in a partial view on the page, the system gets stuck in a loop trying to report the error.

To avoid this, I'm storing a temporary counter of how many times the current action has requested the error page in TempData, but I find the amount of lines and style of the code to get, set and check this variable a bit verbose:

using System.Web.Mvc;

namespace Test
{
    public class ErrorController : Controller
    {
        [ActionName("500")]
        public ActionResult InternalServerError(string aspxerrorpath = null)
        {
            int detectRedirectLoop = (TempData.Peek("redirectLoop") as int?) ?? 0;
            TempData["redirectLoop"] = detectRedirectLoop + 1;
            if((int) TempData.Peek("redirectLoop") <= 1)
            {
                // Check if user is admin or running locally and display error if so
            }
            return Redirect("/GeneralError.htm");
        }
    }
}

Is there a better/prettier/shorter way of doing this?

\$\endgroup\$
  • 4
    \$\begingroup\$ One obvious problem: "redirectLoop" is repeated three times, and should obviously be a const. \$\endgroup\$ – BCdotWEB Sep 1 '15 at 15:16
7
\$\begingroup\$

I suggest the following:

public class ErrorController : Controller
{
    private const string RedirectLoopCounterName = "RedirectLoopCounter";
    private const int MaxRedirectLoopCount = 1;

    private int RedirectLoopCounter
    {
        get { return ((int?)TempData.Peek(RedirectLoopCounterName)) ?? 0; }
        set { TempData[RedirectLoopCounterName] = value; }
    }

    private int IncreaseRedirectLoopCounter() 
    {
        return ++RedirectLoopCounter;
    }

    [ActionName("500")]
    public ActionResult InternalServerError(string aspxerrorpath = null)
    {       
        var isRedirectLoop = IncreaseRedirectLoopCounter() > MaxRedirectLoopCount;
        if(isRedirectLoop)
        {
            return Redirect("/GeneralError.htm");
        }

        // Check if user is admin or running locally and display error if so
    }
}
  • Create a constant for the counter's name as already mentioned by @BCdotWEB
  • Create a property for getting and setting its value
  • Create a method for actually increasing the counters value
  • Additionaly you could replace the 1 by a constant
  • Finally you can replace the condition by a helper variable to document it better
  • I would also invert the if
\$\endgroup\$
  • \$\begingroup\$ oh, I was't sure about that but it shouldn't be a problem to change it ;-) \$\endgroup\$ – t3chb0t Sep 1 '15 at 15:47
  • 1
    \$\begingroup\$ I removed the static keyword. \$\endgroup\$ – t3chb0t Sep 1 '15 at 16:11
1
\$\begingroup\$

I have serious concerns about your use of TempData here: http://www.rachelappel.com/when-to-use-viewbag-viewdata-or-tempdata-in-asp.net-mvc-3-applications

My problem is it assumes your resource requests are single threaded (on a per-client basis), so if you have a client attempting to access more than one resource simultaneously I think your counter gets thrown off.

From my reading of that link I posted it wouldn't know if a subsequent request was in parallel or sequence to your initial 500.

\$\endgroup\$
  • \$\begingroup\$ Therefore, the only scenario where using TempData will reliably work is when you are redirecting that is happening in this case, as all uncaught exceptions redirect the user to error/500 (the webserver does this), and if the exception is also thrown on (part of) the error page, the user gets redirected again to error/500. Before I added the loop counter, this problem would cause a this page is not redirecting properly error in the browser after 30 or so redirects to the same page. +1 for giving me something to test though. \$\endgroup\$ – MrLore Sep 2 '15 at 8:23

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.