Can you find the bug in this code? (THE FIX)

Thanks to everyone for contributing!  It was really neat to read everyone's ideas and see the discussion and review (talking about code is always fun!).  Here is a summary of responses and the "fixed" code. 

If you are interested in the original problem, go here.

 

@drakiula: The idea with the Response.Redirect is that it will stop processing so the else is not needed on line 23.

@JV: Yes, this code could definitely be refactored! :)
And your suggestion would squish the bug.

@rajbk: Nice. Yes, the ThreadAbortException is definitely part of the bug but I don't think the Redirect(url, false) is the best way to go.

@Basgun: Changing line 17-23 as you suggest would make it cleaner but wouldn't fix the bug.

@cibrax,Lee:  Nice. Yes, the ThreadAbortException will cause the catch to fire everytime and redirect the user to the same page.

@Thomas Eyde: Nice. Yes, the refactoring into separate responsibilities would make it much cleaner and also would somewhat inadvertently fix the bug since the redirect would happen outside the catch.

@Zack Jones:  Nice suggestions.  Thanks!

@Hosam Kamel: Yes, good point - there are several ways those method calls could fail (lots of encryption and authentication done within them) so the catch is there as a simple default "then send them here".

Summary:

If you call Response.Redirect(string) or Response.Redirect(string, true), it will throw a ThreadAbortException.  So if you do the redirect inside a generic try/catch then your catch will fail everytime.  As pointed out by several people above, the code could be refactored so the responsibilities are separated out - this means that the Redirect should probably only be done in one place which would mitigate the issue.

I was surprised to see that Response.Redirect throws this exception especially since this exception is not thrown up the Application_Error event so some "magic" must happen within ASP.NET to issue the HTTP location header but not allow the exception to continue as a real exception.  This seems like a misuse of exceptions since a Response.Redirect is hardly an "exceptional" case and in many applications happens somewhere on every page!
That said, it is reasonable to see how Microsoft uses this mechanism to prevent further execution in the page.

Either way, it is a good gotcha to remember!

Here is the "fixed" code:  (although this could still be seriously refactored)

01: private void Page_Load(object sender, EventArgs e)
02: {
03: string redirectUrl = null;
04: try

05: {

06:     AuthenticationInfo authenticationInfo = GetAuthenticationInfo(Request.QueryString["t"]);

07:     Authenticator authenticator = new Authenticator(new LoginProvider());

08:     AuthenticationStatus authenticationStatus = authenticator.Authenticate(authenticationInfo);

09:     if (authenticationStatus.Authenticated)

10:     {

11:         IUser user =

12:             BLUser.Load(authenticationInfo.UserName, authenticationInfo.OrganizationCode, aut
henticationInfo.DomainId);

13:         user.SetNewSessionId();

14:         AuthenticationTokenParser authenticationTokenParser = new AuthenticationTokenParser();

15:         string authenticationToken = authenticationTokenParser.Create(user.UserName, user.OrganizationId,

user.DomainId);

16:         FormsAuthentication.SetAuthCookie(authenticationToken, true);

04:         redirectUrl = FormsAuthentication.GetRedirectUrl(authenticationToken, true);

04:     }

16: }

17: catch

18: {

19:     // If anything goes wrong (encryption error, etc) then just let it fall through to the redirect.

20: }

21: if (redirectUrl == null || redirectUrl.Trim().Length == 0)

22: {

23:     redirectUrl = "~/Home.aspx";

24: }

25: // NOTE: If you do the Redirect inside the try/catch it will get a ThreadAbortException!

26: Response.Redirect(redirectUrl, true);

27: }

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products? 
Take the code test and send your resume along with why you want to join Thycotic to tddjobs@thycotic.com.

2 Comments

  • How else would you want to tear down the thread?

    Would you want to always complete processing after calling redirect?

    The interaction of the thread abort exception and Application_Error is affected by the catch where you don't throw.
    On older sites where I logged all errors from app_error, I had to check for and ignore thread aborts.

  • I guess the model is reasonable given the requirements - as you say "how else would you do it ...". It still feels a little icky though - exceptions are for exceptional circumstances - I don't think redirects count as they are used very frequently in normal page processing. Seems like there should be some other mechanism baked into ASP.NET to handle this situation.

    Either way, it is a good gotcha to be aware of...

Comments have been disabled for this content.