Yesterday I had a live production app start suddenly raising errors after being in fine shape for over a year. In digging into the code to see the guts of the error, I found this:
if (Session["PhaseTypeID"].ToString() == ((int)Enums.PhaseType.Study).ToString())
The session variable isn't being tested for null in this case, it is being used via the toString() call, so when it is null, as in this case, it throws an "Object reference not set exception".
When confronted by code this wrong, there is an overpowering urge to fix it. In my case, I was ready to pull the session vars into the base classes as properties, and add all the error checking there, keeping them contained, and then tidying up the page.
There are two big things wrong with this approach:
- The code has been working fine for over a year, so the root cause of the problem seen is probably not the code, but more likely an environmental change.
- There are no tests around the functionality, so the refactoring would be done without a verifiable way to tell if the changes broke any functionality.
Given those two arguments, the best plan of action is to change as little as possible to resolve the issue. I scrapped my planned changes and investigated the environment, discovering another app that was also running into session errors. The load balancer was no longer setting affinity, so requests could ping-pong from server to server, quite easily losing session state.
So now, the load balancer is fixed, and all is well. The next batch of code changes include setting the web.config to use StateServer for session state, which will help insulate the application should the load balancer misbehave again.
For the planned code changes, we will begin by wrapping tests around the pieces we want to fix, and then start carefully refactoring.