Is it possible using Hashtable as lock object?

Question

I have the following code which "_ht" is an Hashtable that represents cache, and "_isLoaded" represents if it's loaded.

Our system has many processes accessing the "_ht" object, and I need them to wait if it's not loaded.

Does using "_ht" as a lock object is wrong ? Should I use a dedicated object type class member for this scenario ?

It's important to mention that this class is SINGLETON.

private Hashtable _ht = new Hashtable();
private bool _isLoaded = false;

internal Hashtable GetHT()
        {
            if (_isLoaded == false)
            {
                lock (_ht)
                {
                    if (_isLoaded == false)
                    {
                        LoadHt(_ht);
                    }
                }
            }

            return _ht;
        }

if you want me to expend my answer as to why to use a lock object say so, i think it's pretty obvious and if not then you should think about it. anyway lemme know — No Idea For Name, Nov 23 '14 at 08:37
@ohadinho as many said _usually_ a separate `object` is used but until object you're locking against is `private` than nothing wrong to directly use your `Hashtable` (remember you're locking against an instance, not a class and moreover no one else has visibility of that object to lock against it so it's not a source of dead locks). That said I'd **drop this code** all together and I'd use a `Lazy`. — Adriano Repetti, Nov 23 '14 at 08:44
@AdrianoRepetti: note that just because the _field_ is declared as `private`, that doesn't mean the object itself is private. And indeed, in this example the `Hashtable` object is actually publicized (albeit only as `internal`, but still outside the owning class), via the `GetHT()` method. — Peter Duniho, Nov 23 '14 at 08:50
@PeterDuniho I agree but 1) it's internal (as you noted) so it's more _controlled_, 2) locking is used only for construction, 3) to lock against an object returned by something else is - at least - unusual then not as dangerous as locking against `this` and anyway yes, that's why I suggested `Lazy`! — Adriano Repetti, Nov 23 '14 at 11:08

score 10 · Accepted Answer · answered Nov 23 '14 at 08:39

10

You certainly can lock on the Hashtable object, just as you can use any reference type instance in .NET in a lock statement. However, it's generally considered an inferior approach, mainly because it's harder to keep track of how the code is using locking when one or more of the lock objects is available to other parts of the code, where they might use it for locking as well (again, inadvisably, but you'd be surprised at what code people write sometimes).

For locking in general, a separate locking object is preferable. I'll note that in your code example, the _ht should be readonly, and if you add a separate locking object (e.g. lockObj), that should be readonly as well.

That said, the singleton scenario shouldn't be implemented this way at all. Instead, you should either use the CLR's own static initialization, or the Lazy<T> class:

private static readonly Hashtable _ht = InitializeTable();

internal static Hashtable GetHT() { return _ht; }

private static Hashtable InitializeTable()
{
    Hashtable table = new Hashtable();

    LoadHt(table);

    return table;
}

Or:

private static readonly Lazy<Hashtable> _ht = new Lazy<Hashtable>(() => InitializeTable());

internal static Hashtable GetHT() { return _ht.Value; }

private static Hashtable InitializeTable()
{
    Hashtable table = new Hashtable();

    LoadHt(table);

    return table;
}

The latter is useful when you have other members of the type that might be accessed, but you want to make sure initialization of the hash table is delay as long as possible (e.g. if it's possible no code would ever actually access it, so you can avoid initializing it altogether).

(I changed everything to static because you described your scenario as a singleton, and in that case only static members make sense for the code example).

Finally I'll note that the Hashtable class is extremely dated. As a non-generic class, you really should seriously consider upgrading the code to use the now-decade-old generic type. The Dictionary<TKey, TValue> class is the most direct replacement, but people sometimes use Hashtable as a simple set, for which the HashSet<T> data structure would be more appropriate.

answered Nov 23 '14 at 08:39

Peter Duniho

68,759
7
102
136

Only exception to the answer would be "just as you can use any reference type instance in .NET" - except strings. You cannot use strings effectively as a lock object as they are immutable so their reference is shared amongst all other code which has the same string value. – PhillipH Nov 23 '14 at 08:49
1

@PhillipH: sorry, your statement is incorrect. First, the fact that `string`s are immutable in no way implies that all `string`s that are equal are in fact the same object (by default, only `string` literals are interned). Second, even if it did imply that, that doesn't preclude using some `string` reference in a `lock` statement. It may be doubly unwise, but it's certainly possible. – Peter Duniho Nov 23 '14 at 08:54
Thanks a lot for your solution. I have a question regarding it: If 10 threads trying to execute "GetHT()" and the Hashtable is not loaded. DOES ONLY ONE THREAD WOULD EXECUTE InitializeTable() BECAUSE IT'S STATIC ?? – ohadinho Nov 23 '14 at 09:13
1

@ohadinho: yes. Because it's static _and_ it's initialized using the C# field initializer syntax, the CLR provides a guarantee that it will be initialized before the first use of the field, and that it will be initialized only once. (Static fields alone don't get this guarantee...you have to provide an initializer and then mark the field as `readonly` to ensure it won't be reinitialized somewhere else. And to be clear, you get the initialization guarantee just with the initializer, the `readonly` is just to make sure no other code changes it later). – Peter Duniho Nov 23 '14 at 09:15
@PeterDuniho: so readonly does all the magic. Got it! thanks! – ohadinho Nov 23 '14 at 09:42
1

@ohadinho: sorry, no...I was trying to make that clear but I guess I didn't. It is specifically _not_ the `readonly`. That's just there to make sure no other code overwrites the initialized value. The "magic" comes from having the initializer, i.e. the `= InitializeTable();` part. It would work just as well without `readonly`, as long as you're careful to never assign the value again later. – Peter Duniho Nov 23 '14 at 09:46
@PeterDuniho ok I get it, but can I use a static ctor as the initializer ? private static readonly Hashtable _ht; static ClassName() { _ht = new Hashtable(); LoadToHashTable(); } – ohadinho Nov 23 '14 at 10:06
1

@ohadinho yes, you can use static ctor. There is a nice Eric's post about that but here do you need a static field? In your post it's not (and even if you get a _thread safe_ initialization it'll change (a lot!) code behavior. – Adriano Repetti Nov 23 '14 at 11:11
@ohadinho - you are correct, however I have seen instances where programmers delclare ' private string lockObject = ""; ' as their standard method for declaring a lock object - and this is unlikely to behave as expected. Consequently despite the rules for interning being as you state, I still considering it a "code smell" to see strings used as lock objects. However, we'll leave as "unwise" for the future reader :o) – PhillipH Nov 23 '14 at 16:08

score 2 · Answer 2 · answered Nov 23 '14 at 08:28

2

if you want the first thread that comes here initiate it it's ok. but usually you use a lock object for that.

private Hashtable _ht = new Hashtable();
private bool _isLoaded = false;
private object lockObj = new object();

internal Hashtable GetHT()
{
    if (_isLoaded == false)
    {
         lock (lockObj)
         {
             if (_isLoaded == false)
             {
                 LoadHt(_ht);
             }
         }
     }

     return _ht;
}

answered Nov 23 '14 at 08:28

No Idea For Name

11,411
10
42
70

Double-checked locking should generally be avoided, if for no other reason than that it's easy for people to implement it incorrectly (as you have here). It's also unnecessary in .NET, because .NET offers at least two other superior implementation choices. See http://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Microsoft_.NET_.28Visual_Basic.2C_C.23.29, http://stackoverflow.com/a/394932/3538012, and http://csharpindepth.com/Articles/General/Singleton.aspx for some additional discussion on the topic. – Peter Duniho Nov 23 '14 at 08:46
This isn't singleton pattern, however this kind of double checked locking is broken. If `_ht` is instantiated inside the `LoadHt` method, then processor can reorder `_ht` and `_isLoaded` resulting in returning `null`. Refer http://joeduffyblog.com/2006/01/26/broken-variants-on-doublechecked-locking/ – Sriram Sakthivel Nov 23 '14 at 08:50
@SriramSakthivel: actually it's broken worse than the example in the Duffy blog article. In this example, the `_isLoaded` variable is never even assigned, so it doesn't even get the chance to stumble over the problem Duffy's talking about. :) But yes, the lack of a volatile read is also a problem here. This is why I always strongly advocate against trying to implement one's own double-checked lock. Relatively few programmers get it right, and there are easy alternatives in .NET to use. – Peter Duniho Nov 23 '14 at 09:02
@PeterDuniho I assumed `_isLoaded` will be set to true inside `LoadHt` method. If that assumption is correct, then it is only the matter of non volatile read, isn't it? – Sriram Sakthivel Nov 23 '14 at 09:07
@SriramSakthivel: if the assumption is correct, then I agree that the only other problem I'm aware of is the non-volatile read. I'm hesitant to claim that _is_ the only problem, because as experienced as I am with general-purpose multi-threaded code, I am not an expert as compared to someone like Duffy or the folks who are writing the actual high-performance thread-safe types in .NET like the concurrent collections. That's why I leave the tricky stuff to them. :) – Peter Duniho Nov 23 '14 at 09:12

Max Brodin · Answer 3 · 2014-11-23T08:55:08.300

1

Object itself is not locked (protected) to begin with. The reference used in the lock keyword is used to mark or tag a section of the code that should not run simultaneously with any other (or same) section of code that used the same object reference. It does not actually affect the object itself. So answer is yes you can use your existing HashTable instance in lock statement.

Best practice though is to define a private object to lock on, or a private static object variable to protect data common to all instances.

private Object thisLock = new Object();

Edited Thanks @PeterDuniho for pointing out

edited Nov 23 '14 at 08:55

answered Nov 23 '14 at 08:33

Max Brodin

3,903
1
14
23

Thanks for fixing the statement about "new object". I would also suggest you clarify "section of code that used the same object reference". As you probably do know, it only protects sections of code that use the same object reference _in a `lock` statement_. Any code that uses the object without using also a `lock` statement will remain unsynchronized. I think you could simply word it as "...that uses the same object reference in a `lock` statement", and that will ensure against any misunderstanding. – Peter Duniho Nov 23 '14 at 08:58

Is it possible using Hashtable as lock object?

3 Answers3