returning when inside a lock - is it safe?

[Brandon Haase]

I've run into a situation I haven't seen before, and was wondering if someone with a bit more threading experience would like to chime in. The class IsoCurrencyInfo maintains a collection of IsoCurrencyInfo singletons. The instances of IsoCurrencyInfo class themselves are read-only once created, so by themselves are thread-safe. The trick is keeping the ArrayList container in line. I think I have it nailed, but in doing so I am executing a return statement while inside a lock(). The code works very well under heavy concurrent load :cool:, but I can't help wondering "Is this safe?" Is there a better way to structure this method to optimize the locking? I have adapted the Singleton implementation pattern[^] from Microsoft Patterns & Practices[^].

public static IsoCurrencyInfo GetInstance(string isoCode)
{
IsoCurrencyInfo newICI = null;

if(null == \_iciRegistry)
	lock(syncRoot) 
		if(null == \_iciRegistry)
		{
			\_iciRegistry = new ArrayList();

			newICI = new IsoCurrencyInfo(isoCode);
			\_iciRegistry.Add(newICI);
		}
if(null == newICI) 
	lock(syncRoot) 
	{
		foreach(IsoCurrencyInfo existingICI in \_iciRegistry)
			if(existingICI.AlphaCode == isoCode)
				**return existingICI;**

		newICI = new IsoCurrencyInfo(isoCode);
		\_iciRegistry.Add(newICI);
	}
return newICI;

}

Thanks! B

[Heath Stewart]

It's completely safe. The lock keyword translates to the following (verifiable if you use ildasm.exe or some other disassembler / decompiler to view the compiled code):

Monitor.Enter(syncRoot);
try
{
// Do stuff
}
finally
{
Monitor.Exit(syncRoot);
}

The finally block is always run - regardless of a return or a throw - except when Environment.Exit is called (which unloads the process and the CLR completely, thus it really won't matter that the lock wasn't released because the OS will reclaim any process resources).

-----BEGIN GEEK CODE BLOCK----- Version: 3.21 GCS/G/MU d- s: a- C++++ UL@ P++(+++) L+(--) E--- W+++ N++ o+ K? w++++ O- M(+) V? PS-- PE Y++ PGP++ t++@ 5 X+++ R+@ tv+ b(-)>b++ DI++++ D+ G e++>+++ h---* r+++ y+++ -----END GEEK CODE BLOCK-----

[scadaguy]

Brandon, Returning inside a lock is okay. I don't necessarily see anything incorrect about your implementation. However, the double-checked locking pattern is essentially useless as you have used it since you'll always be aquiring at least one lock anyway. I would suggest the following code...

public class IsoCurrencyInfo
{
private static Hashtable _iciRegistry = new Hashtable();

public static IsoCurrencyInfo GetInstance(string isoCode)
{
    if (!\_iciRegistry.Contains(isoCode))
    {
        lock (syncRoot)
        {
            if (!\_iciRegistry.Contains(isoCode))
            {
                \_iciRegistry.Add(isoCode, new IsoCurrencyInfo(isoCode));
            }
        }
    }
    return (IsoCurrencyInfo)\_iciRegistry\[isoCode\];
}

}

It is important to realize that the Hashtable is the only IDictionary collection that will work as I have used it. This is because the Hashtable can support multiple readers and one writer simultaneously. Likewise, you won't be able to use the same trick with an ArrayList. In this implementation the lock is rarely acquired. Brian

[Brandon Haase]

Thank you for a solid example. I didn't think of checking my decompiler's output.

[Brandon Haase]

Bravo! Based on your code and some thought, I ended up restructuring the entire class. It is smaller, tighter, and a lot faster. My test suite execution time dropped from an average of more than 500ms to less than 200ms; it drops to less than 1ms when I pre-load the singletons. That lock-loop was killing me. Thanks! B