?

Log in

No account? Create an account
 
 
31 July 2009 @ 12:31 pm
Ugly code  
Well i found one of the problems with the code i'm working on, though unfortunately not the big nasty ones i'm still dealing with from yesterday.

Once i hacked my way around the XML/Enterprise stuff everything seemed to be working fine, except occasionally the initialization of the static security mapping would throw an error saying it tried to add a duplicate value to the list. The code in the second was something like (in abbreviated form obviously):

if (mapping.Count == 0)
{lock (mapping)
{add stuff to mapping}}

I double checked by adding another static variable in the inner loop to keep track of how often it got initialized, and sure enough it was managing to get inside that code block twice. When i checked "lock" on MSDN it turns out that if something else tries to lock the object it just blocks and waits until the object is unlocked, then continues on its merry way. So in this particular case it doesn't prevent a collision, it just delays it.

so i can either move the mapping.Count check inside the lock, so only one thread can be checking it at a time, or just copy/paste the same check inside the lock loop as well. MSDN doesn't say, but i presume a lock action is more computationally intensive then just checking the Count, and since 99% of the time the first check is enough to prevent the collision without resorting to getting a lock it would make more sense from that perspective to check it twice, both before and after the lock. However repeating the .Count check twice like that just _looks_ dumb. Maybe i can find a variant of lock that skips instead of blocks?

And no, i didn't write this code originally, i'm just supposed to be moving it from the Enterprise project to the local project. I'm not sure why it (presumably) worked in the old location without the same error, and i'm kinda reluctant to change the code too much, like trying to rewrite it so the initialization happens somewhere else so i'm sure it can only get called once. Especially not when i'm still obviously unfamiliar with most of the details of C# =P
 
 
Current Mood: happyhappy?
 
 
 
Marvin Spencermarvinalone on August 1st, 2009 04:35 pm (UTC)
Isn't that what locks do all the time? Isn't that what they're for? I don't write much C# either, but I think you can do something like this:
// (statically) initialize some_int to 0
while(true) {
    int o = System.Threading.Interlocked.CompareExchange(some_int, 1, 0);

    if(o == 0) {
        // initialize stuff
        o = 2;
    } else if(o == 1) {
        Sleep(0);    // give up the current timeslice
        continue;
    } else {
        break;
    }
}
This will initialize only once, and wait in a spinlock while it does so. If your initialization takes longer than no time at all, you may want to sleep for a little longer than 0ms.
Marvin Spencermarvinalone on August 1st, 2009 04:36 pm (UTC)
Oh, and it doesn't use an expensive lock to do all this. That's more or less the point :-)