Otdfbt

I recently read a book about system software.
There is an example in it that I don't understand.

volatile T* pInst = 0;
T* GetInstance()

 if (pInst == NULL)
 
 lock();
 if (pInst == NULL)
 pInst = new T;
 unlock();
 
 return pInst;

Why does the author check (pInst == NULL) twice?

When two threads try call GetInstance() for the first time at the same time, both will see pInst == NULL at the first check. One thread will get the lock first, which allows it to modify pInst.

The second thread will wait for the lock to get available. When the first thread releases the lock, the second will get it, and now the value of pInst has already been modified by the first thread, so the second one doesn't need to create a new instance.

Only the second check between lock() and unlock() is safe. It would work without the first check, but it would be slower because every call to GetInstance() would call lock() and unlock(). The first check avoids unnecessary lock() calls.

volatile T* pInst = 0;
T* GetInstance()

 if (pInst == NULL) // unsafe check to avoid unnecessary and maybe slow lock()
 
 lock(); // after this, only one thread can access pInst
 if (pInst == NULL) // check again because other thread may have modified it between first check and returning from lock()
 pInst = new T;
 unlock();
 
 return pInst;

See also https://en.wikipedia.org/wiki/Double-checked_locking (copied from interjay's comment).

Note: This implementation requires that both read and write accesses to volatile T* pInst are atomic. Otherwise the second thread may read a partially written value just being written by the first thread. For modern processors, accessing a pointer value (not the data being pointed to) is an atomic operation, although not guaranteed for all architectures.

If access to pInst was not atomic, the second thread may read a partially written non-NULL value when checking pInst before getting the lock and then may execute return pInst before the first thread has finished its operation, which would result in returning a wrong pointer value.

I assume lock() is costly operation. I also assume that read on T* pointers is done atomically on this platform, so you don't need to lock simple comparisons pInst == NULL, as the load operation of pInst value will be ex. a single assembly instruction on this platform.

Assuming that: If lock() is a costly operation, it's best not to execute it, if we don't have to. So first we check if pInst == NULL. This will be a single assembly instruction, so we don't need to lock() it. If pInst == NULL, we need to modify it's value, allocate new pInst = new ....

But - imagine a situation, where 2 (or more) threads are right in the point between first pInst == NULL and right before lock(). Both threads will to pInst = new. They already checked the first pInst == NULL and for both of them it was true.

The first (any) thread starts it's execution and does lock(); pInst = new T; unlock(). Then the second thread waiting on lock() starts it's execution. When it starts, pInst != NULL, because another thread allocated that. So we need to check it pInst == NULL inside lock() again, so that memory is not leaked and pInst overwritten..