The “1-2-3 Click !!!” test …
These past couple of weeks have been really hectic at work. We’ve been working 60-hour weeks, chasing after new features that we needed … yesterday (sic), bug fixes etc, etc.
Well, anyway, the point is that we started looking at the peformance of our application here, and delved a little deeper on concurrency and load issues. So, I thought I’d share some of the experience here …
1. Caching do’s and dont’s …
Our Data access code is based almost on it’s entirety on Typed DataSets. It makes it easier to persist new, updated or deleted rows in the database, just by updating the DataSet we’re working on. A core part of our Data Access library used to create the necessary SqlCommand objects for each operation, and automatically execute them being passed a DataSet instance. It would check for updated, new and deleted rows, and call on those Commands accordingly.
First time I saw this code, I noticed that the guy who wrote it cached those SqlCommand objects in a Dicitionary, to save the overload of instantiation. Here’s a code snippet that illustrates that …
…
If Not m_sqlobjCache.Contains(strCommandName) Then
Dim sqlcmdToAdd As SqlClient.SqlCommand
If blnWithDaMapping Then
sqlcmdToAdd = sqlHelper.SqlCommand_CreateForDataset(New SqlClient.SqlConnection(DBConnectionString), strCommandName, True)
Else
sqlcmdToAdd = sqlHelper.SqlCommand_Create(New SqlClient.SqlConnection(DBConnectionString), strCommandName, True)
End If
m_sqlobjCache.Add(sqlcmdToAdd.CommandText, sqlcmdToAdd)
Return sqlcmdToAdd
…
One of our first checks was a bit ludicrus … a couple of us “sychnronized” ourselves, and clicked on the same button/link on the web page, to test the code for concurrency ( it was really out-of-this-world to walk into a room and hear 3-4 guys yelling “one-two-three-GO!!![click]” 
).
Well, 3 out of 4 times, one of us would get an error, saying “Key already exists in the Collection” or something like that, and fail. What gives ???
Well … the object containing the code above is a Singleton. There’s only a single instance of it, serving all calling threads. What does that mean in our case ? It means that although the code checks whether the given key exists in the collection, and number of different threads can be running simultaneously, and each one of them can be in any one of those lines of code above, at the same time, on the same running instance.
In plain English, two threads that have BOTH checked whether the key existed in the Collection, could try to add the same key with a time difference of nano-seconds. Nothing locks those lines of code so that no two threads can enter them ( we call something like that a ‘critical section’ in formal terminology ) and modify the collection at the same time. So, we needed … locking.
Enter version 2 of the same code snippet …
…
SyncLock (m_sqlobjCache)
If Not m_sqlobjCache.Contains(strCommandName) Then
Dim sqlcmdToAdd As SqlClient.SqlCommand
If blnWithDaMapping Then
sqlcmdToAdd = sqlHelper.SqlCommand_CreateForDataset(New SqlClient.SqlConnection(DBConnectionString), strCommandName, True)
Else
sqlcmdToAdd = sqlHelper.SqlCommand_Create(New SqlClient.SqlConnection(DBConnectionString), strCommandName, True)
End If
m_sqlobjCache.Add(sqlcmdToAdd.CommandText, sqlcmdToAdd)
Return sqlcmdToAdd
End SyncLock
…
That seemed to do the trick. No more duplicate key errors in our 3-man 1-2-3-Click test 
Until, next day, we decided to put some more people on the test ( I know I know, there’ s a thing called Application Center Test to do stuff like that but .. it’s more fun having the whole company yell “1-2-3-GO!!!” at 11:00 in the morning ). So, we started runnig around the company, getting more and more people involved. All in all, we ended up with about 15 people trying to concurrently run the same query, with different parameters. No-one got a duplicate key error.
But some of them got a “There is already a DataReader running on the specified Connection” error instead! Oh boy .. what now ???
Well … after an even closer inspection of the code, we see that …
sqlcmdToAdd = sqlHelper.SqlCommand_CreateForDataset(New SqlClient.SqlConnection(DBConnectionString), strCommandName, True)
… that means, that those Commands are being cached complete, with their SqlConnection. Furthermore, that means that this specific SqlConnection will get used every time this particular command is being executed … and if two people try to execute the command with a nano-second time difference, the first run hasn’t had the time to execute and return the data before the next one starts ! Hense, the error that in other words says “i’m trying to do something on this Connection right now, I can’t start doing something else as well”
Furthermore, that means that there is no exploitation of the built-in Connection Pooling provided by .NET, a limited number of possible concurrent connections and a whole slew of other little nuances that might in fact have a tremendous effect on the scalability of our application …
So, after a little chat, we gave up on the caching feature of SqlCommand objects. It just wasn’t worth the hassle to save the load of instantiation, when that would cause such scalability problems. The quickest remedy was to forget caching alltogether.
Now, if we re-wrote the caching class, we could store those SqlCommands without the included SqlConnection. We could be providing the SqlConnections just prior to execution, but then we thought that it might cause other concurrency issues later on, so … we deferred the whole issue for later.
2. Beware of sync blocks bearing gifts …
A couple of days afterwards, I’m inspecting my own code for sync errors. One very basic component in the processing pipeline regarding activities and wizards within our system start by calling an HttpHandler (the equivalent of Java Servlets or CGI for the .NET world).
Now, in your handler, you can specify whether that Handler is reusable - in other worlds you can choose whether you want a new instance of the handler to serve every new request coming into the system , or whether IIS can “pool” instances in some manner.
Now, originally I’d gone for the “new instance” approach, thinking that the Handler is a pretty lightweight object with just 4 int properties, so … I left it at that. Here’s what the code looked like:
public void ProcessRequest(HttpContext context){
RequestBean.InitFromHttpRequest(this);
// Now init an instance ..
try
{
ActivityFactory.InitActivity(ContractActivityID, ContractID, CallPhaseEnumID, CallPhaseID);
}
catch(ApplicationException e_app)
{
// We’ll see ..
throw e_app;
}
catch(System.Threading.ThreadAbortException e_ta)
{
// Ignore Thread AbortExceptions
}
catch(Exception e)
{
// We’ll see …
throw e;
}
}
public bool IsReusable {
get {
return false;
}
}
However, later on I reconsidered, thinking that I was leaving a - small, it’s true - overhead on my server, without any real need for it … so, version two fo the code looked a bit like this:
public void ProcessRequest(HttpContext context){
lock(this){
RequestBean.InitFromHttpRequest(this);
// Now init an instance ..
try
{
ActivityFactory.InitActivity(ContractActivityID, ContractID, CallPhaseEnumID, CallPhaseID);
}
catch(ApplicationException e_app)
{
// We’ll see ..
throw e_app;
}
catch(System.Threading.ThreadAbortException e_ta)
{
// Ignore Thread AbortExceptions
}
catch(Exception e)
{
// We’ll see …
throw e;
}
}
}
public bool IsReusable {
get {
return true;
}
}
Now, I’m thinking that this is OK. Every request might as well get served by a few instances being recycled by the server as required. I’ll work that way … and indeed, it seemed to work ok on a couple of test runs I did.
After a while however, I start seeing very weird results. The handler would redirect me to irrelevant activity wizards, although the data was corrent ion the RDBMS, and the parameters passed to it made sence … and then it hit me.
I’m not playing around with a new instance every time. All the property values being set, will remain there until I explicitly nullify or re-initialize them! Doh .. you might think. Well, it wasn’t as straightforward to realize as you might think at the time … so, I introduced a method that re-initialized the properties, and called that before releasing the lock on the handler, and voilat !!! The handler was now working much faster, and correctly … here’s the final version of the code ..
public void ProcessRequest(HttpContext context){
lock(this){
RequestBean.InitFromHttpRequest(this);
// Now init an instance ..
try
{
ActivityFactory.InitActivity(ContractActivityID, ContractID, CallPhaseEnumID, CallPhaseID);
}
catch(ApplicationException e_app)
{
// We’ll see ..
throw e_app;
}
catch(System.Threading.ThreadAbortException e_ta)
{
// Ignore Thread AbortExceptions
}
catch(Exception e)
{
// We’ll see …
throw e;
}
finally {
ReInitProperties();
}
}
}
public bool IsReusable {
get {
return true;
}
}
Notice the finally { … } block, where I re-initialize the property values to zero. Also note that this is done within a finally {…} block, meaning that it will get executed no matter what happens before, and only after that the lock will be released.
So, that’s that for now … I’ll be sure to post such coding anecdotes as often as I can for your (and mine) viewing pleasure 