What's wrong with this code?

Published Wed, Oct 13 2004 18:34 | William

Well, if you see someone post a bug in the newsgroup and you've never seen there name there before, chances are it's not a bug.  The signal to noise ratio on alleged bugs is quite low.  So, someone posted this today claiming that it's a bug or if it's not, it's a terrible thing that should be documented.  I know most of the people that read this blog are pretty astute, so can you spot the bug?

using System;
using System.Threading;
using System.Data;
using System.Data.SqlClient;

namespace SqlCommandBug
{
class Class1
{
[STAThread]
static void Main(string[] args)
{
new BugReproducer().Reproduce();
}

public class BugReproducer {
public static SqlCommand cmd = new SqlCommand();
public static SqlCommand cmd2 = new SqlCommand();

public BugReproducer() {
cmd.CommandText = "Select * from Parameter";
cmd2.CommandText = "Select * from Application_Curve";
}

public void Reproduce() {
for (int i = 0; i < 2; i++) {
new Thread(new ThreadStart(this.DoIt)).Start();
}
}

public void DoIt() {
SqlConnection conn = new
SqlConnection(System.Configuration.ConfigurationSettings.AppSettings["ConnectionString"]);
conn.Open();
SqlTransaction t1 = conn.BeginTransaction();

lock (cmd) {
     cmd.Transaction = t1;
     cmd.Connection = t1.Connection;
     IDataReader reader = cmd.ExecuteReader();
     while (reader.Read());
     reader.Close();

}
lock (cmd2) {
     cmd2.Transaction = t1;
     cmd2.Connection = t1.Connection;  //Note from Bill - this is the supposed “Bug“ because it throws an exception
     IDataReader reader2 = cmd2.ExecuteReader();
     Thread.Sleep(3000);
while (reader2.Read());
     reader2.Close();
     }
   t1.Commit();
}
}
}
}

Now the 'solution' of course that he found was that if he sets the connection and command to null in between them and reinstantiates everything, the problem is fixed. Here's a quote “It seems this either is a bug or if not, the documentation should be updated to say to explicitly set the Transaction and Connection properties of the command to null after the Reader is closed...

Yes my friends, setting the transaction to null before calling .Commit is the 'solution' hence it's a bug and Microsoft f****ed up by not documenting it.

In the context of the code, it would seem like a Dad yelling at God because he didn't get a warning message when his son was born telling him that leaving the keys to the new Porsche, the Amex card and a stocked liquor cabinet when the kid's 16 years old, before leaving for the weekend , may not be a good idea.  Yessir, let me gas the old 911 up and make sure I pay off the balance on the Amex, and oh yes, maybe I should hit the liquor store and load up on everything before leaving.

Filed under:

Comments

# William said on October 13, 2004 9:10 PM:

I don't know enough about C# really to say but if that was Java I'd tell him he needs to close and null his connection before he re-uses it like that. Also opening a connection of any kind without some kind of try catch is bad juju. Scott told me the other day that C# supports a syntax where you can say

using(whatever connection){
}

and it will close and null out your connection in a try catch finally block behind the scenes.

It was here:
http://odetocode.com/Blogs/scott/archive/2004/10/04/526.aspx#FeedBack

So since Java and C# are supposed to be very close I'd tell him to try closing and nulling his connections before he re-uses them.

Am I close?

# William said on October 13, 2004 9:55 PM:

Don't know Bill, this looks like a bug to me...

As much as I hate multi threaded scenarios he is locking the affected object and we should not be throwing inside his lock statement. I should note that he is not setting the transaction to null, he is nulling the commands transaction and connection properties.

# William said on October 13, 2004 10:14 PM:

Sorry, I can't see the "setting to null thing". If I was forced to write this code it would have created a SqlTransaction inside the first lock(cmd) { } block and created another SqlTransaction inside the second lock(cmd2) { } block. That means that there is a Commit per lock() block.

Also, how many connections do you want to open? That one calls for an immediate refactor. How many connections can one have per process? I need to look that one up.

And when you are writing a symphony like this one, make it symmetrical. Write 'cmd1' and 'cmd2' instead of 'cmd' and 'cmd2'. And 't1' without a 't'? Come on, is this a hint to a T1 Internet connection. Where is the introduction to the 't' theme?

Just my 2 Euro cents

# William said on October 13, 2004 11:26 PM:

Angel:

The second command is totally unnecessary though - and if you do it with one command, just change the command text then it works fine. There's no need for the second command object at all and if you can do it all under one command/connection/transaction what do you gain by not doing it?

You're right, he's settting the transaction property to null not the transaction and I'm pretty sure that's why it then works, b/c the reference isn't live anymore.

I guess here's my ultimate point.... you can get into a lot of trouble with threading particularly when you have some bizarre construct like this. If you create the commands locally then the thread synchronization never even becomes an issue because you're not using the same object. I just wouldn't expect this code to work reliably with anything considering how things are referenced.

# William said on October 14, 2004 12:05 AM:

My other question would be why the wierdness? Normally you do connection stuff ( I don't know about db connections but for http type stuff ) one per thread IE:

main thread/object -> spawns thread( creates it's own single connection retrieves data )
|
main thread/object waits
|
main thread/object <-spawned thread locks some method in main object then updates main object with retrieved data via locked method then notifies main object that it may continue. Closes and destroys connection local to spawned thread.


Why would you pass the connection itself into the thread? I know on the small device stuff I do that will deadlock the device. All connection stuff should be done in it's own thread so you don't hang your main process.

This is also real scary:
Thread.Sleep(3000);

What is this sleeping for and how is he verifying that what he is waiting on for 3 seconds actually occured? Again I have no idea if this is standard C# type threading and maybe in C# it's ok to do that but in all the multi-threaded stuff I work with Sleep is a huge warning flag that somebody doesn't know what they are doing.

Somebody needs to explain to him the whole concept of producer /consumer threading.

Producer locks does it's thing while Consumer waits, Producer finishes and updates the data, then notifies Consumer, Consumer awakens and uses data produced by Producer. The very first most basic threading class I ever had made you write scenarios like that until you wanted to puke but apparently it's neccesary if people are writing wierdness like that.

# William said on October 14, 2004 12:43 AM:

Andy:

I'm with you, this is sh1t code. Why create the static variables but have everything else as instance objects. I agree about the deadlocks too - it's not like it ever takes more than a split second to open a connection and execute a command. The whole thing is so damned wierd and if you write really weird code you shouldn't be surprised when you get weird behavior - and when you thread really weird code - well, the Porsche analogy comes into play.

# William said on October 14, 2004 3:17 PM:

Bill,
I am afraid I have seen this style of code before, and I don't know enough to say that it is wrong (though I do not like it!)

In the code above substitute the bland looking sql with a parameterized query with 100+ parameters (I kid you not!). Does it still make sense to create and dispose the command in each method? I can understand people not wanting to throw this resource away, specially since the api allows moving a command from one connection to another.

I think that locking is evil, that even if you can get it to work slightly faster today you will regret this architecture going forward as you switch to a more powerfull machine or to use more threads that cause more contention and I would certainly not recomend it. I would not be surprised to hear that under certain circumstances the code above could be faster than non-locking code though.

# William said on October 14, 2004 3:50 PM:

Apart from the anomalies that previous readers have already commented – Like Connection not closed, unnecessary Thread sleeps; I still feel this code is valid (though I don’t like it!) and we should not throw an exception as Angel mentioned. The code synchronizes the objects (cmd and cmd2) that are not thread-safe which it uses between threads. Also, the Connection and Transaction objects are new objects that are created on every thread.

# William said on October 14, 2004 4:55 PM:

Angel:

At least there were parameterized queries - I've seen some pretty long dynamic sql statements but not 100 values.

Here's my gripe with it. IN the dudes post he was being a smart ass about it. THe part about telling you that you should set the values to null is particularly what annoyed me. Maybe I'm being too hard on the dude but from any angle that you look at it from, it's a flawed design.

Assume he doesn't want to waste resources so that's why the variables are static. From an efficiency point of view, what value does having two commands where one will work provide? Just picture the above code,except that it only has one command, switches the commandtext. Logically it would flow much better, it would start to make sense, you'd get rid of an object and a lock statement to boot.

Next, think of the scenario here. Two of the exact same sets of Select statements executed back to back. I know this may just be his example but the way it's structured I really doubt it since the command text isn't being passed in (which would then cause it not to work). What benefit would running the same query in a loop have? I mean, if you really needed to reference it twice, why not just put the second set of code that you needed the reader for and do both things in a single loop - or, use a DataTable, fill it once and reference it from wherever you needed it? And what value is the Transaction serving? What is commit doing here?

Ok, so maybe it's on insert statements? Well, there's no Rollback so that isn't the case. Assuming that is is though, since there isn't a rollback, what value is the transaction serving assuming it wasn't being used on a Select statement?

So to be fair, maybe it shouldn't throw an exception. But him being a smart ass about it, whenever you have code that no one at Microsoft probably ever dreamed someone would write isn't cool. And this is about the 20 trillionth time I've heard someone scream bug in a threading situation where the code doesn't do what it wants them to. I've used threading and transactions in ADO.NET for about three years now and not once have I had an issue with it caused by anyone other than ME.

And not one person has addressed my issue of taking the code, printing it out, taking four steps back and seeing a picture of Satan. It's him, with horns and everything ;-)

# William said on October 14, 2004 6:18 PM:

Bill,
I have seen too many threads that have the "M$ sux because I don't know how to code" theme to be able to ignore what you are saying (I can't stop laughing at the horns imagery!) I truly appreciate that people like you are happy to point out that, just maybe, if they had not shot themselves in the foot it wouldnt hurt so much *grin*.

Unfortunately in this case I believe that we do sux, just because the code has horns don't mean it shouldn't work. I suspect that this bug is reproducible without threads and it is directly my fault.

Time to eat humble pie.

# William said on October 14, 2004 6:57 PM:

Don't even get me started on the M$ thing. I can think of no better way to parade the fact they're a LOSER than having Micro$oft, Bill Gate$ or $teve Ballmer in an email. I mean, if it was funny at some point in time it had to have been like 20 years ago. Of all the things to make fun of someone you don't like for, being more succesful than you doesn't seem like one of them. There's a dude in the .NET General group that's like Super Tr0ll (even the Linux guys hate him and he's one of their biggest advocates) and every single post it's Bill Gate$, $Teve Ballmer or Micro$oft. He actually shies away from the last one and uses MicroTroll$ and WinTroll$ even when he's referring to an individual. And you know he drives a old Yugo around, has poor hygiene and on the off chance he has a girlfriend or lifestyle partner, it's only because they have self-esteem issues.

Besides, since I don't have any children I've adopted ADO.NET as my child and some dork insulting it gets me mad. Sure, neither Microsoft nor ADO.NET is perfect, but neither am I and neither are they. And I'll guarantee you that the dude's command of the language isn't so high that he's got room to talk - he stumbled into it by creating code that Cross Dresses as a freak of nature. I'll guarantee that the laziest dumbest dude in your department probably has 20 IQ points on most critics and probably works 20 hours a week longer. I guess the anti-MS stuff just particularly irritates me sometimes.

Hey, I just realized something "funny" about your crew..... Angel $aenz-Badillo$, $u$hil Chordia, Pablo Ca$tro, David $ceppa, by Anti-MS Dork Logic, just by spelling out a few of your names I just wrote the funniest $entence is recorded history. And the sad part is, if I posted this in COLA, there'd be a lot of laughter.

# William said on October 17, 2004 9:11 AM:

sorry guys & Bill... just thought i'd add this in: http://www.microsoft.com/security/incident/aspnet.mspx

vulnerability in forms and windows authentication..easy fix though.

Search

This Blog

Tags

Community

Archives

News

My other sites

Cool Stuff

Book Stuff

Security

ORM

Data Access

Funny Stuff

Compact Framework Stuff

Web Casts

My KnowledgeBase Articles

My MVP Profile

Design Patterns

Performance

Debugging

Remoting

My Fellow Authors

My Books

LINQ

Misc

Speech

Syndication

Email Notifications