CareerCup

edited, clarified points and added the check for usage error (post condition)

I assume c++, the code calls lock/unlock every time (so current code is not optimized for lockless which it probably should be, depending on usecase)

1) increment, decrement are not necessarily atomic, for example not if multi core, so it must be inside of lock or you should state that it doesn't run on multi core environment (which means today, it doesn't run on most smart phones, pcs, ...) not doing that is an error, you better don't guess on this.

2) returning the value of the variable outside of the lock, will eventually allow changing the value because the code may be interrupted between the unlock and the return and thus the return value is not deterministic. So you need to copy the value of the variable while in the critical section (locked)

3) the original code has no unlock when returning 0, assuming the "free" function unlocks the lock is very bad practice because it would mean very bad design: normaly, one creates thread safe methods to be used from outside the class but inside the class the methods assume a locked object, so you never need to call nested lock functions which eventually would unlock to early if not used properly... so, no, assuming free unlocks the lock, would be not a good sign.

4) what often happens is the code is not exception safe, one can argue, that the only place an exception can be thrown is in the "free" method and then it doesn't really matter because something really goes wrong. I would still want resources be unlocked to avoid further troubles, for example when logging etc...

5) one should really check if Release is called when m_refCounter is 0 because that would mean someone uses the referenc counting mechanism wrong, and that should be at least asserted.

Release()
{
	m.lock(); // 1)
        assert(m_refCounter > 0); // 5)
	try //4)
	{
		m_refCount--;
		int result = m_refCount; // 3)
		if (m_refCount == 0)
		{
			free(this);
		}
		m.unlock(); // 3)
		return result; // 2)
	} 
	catch(...)
	{
		m.unlock(); // 4)
		throw;
	}
}

one intersting question is, how to optimize relatively expensive mutex lock / unlock if lots of "Realease" are called with m_refCount > 1 on multi core processors. While one can do something for single core, its realy hard to get it right on multi core due to caching because one can't assume memory access to be serialized between cores...

return 0 without unlock call?

That was my initial guess, but I think the call to free(this) may take care of that.
My second guess is that we should decrease m_refCount before locking

i guess decrement should be done after taking lock.

I'm pretty new to coding. I'm pretty sure though, that a function needs a cololon and the m.unlock() is missing a Collon. Shouldn't it be m.unlock(); ?

I see few issues in this code.
1. m_refCount-- is being updated outside the lock. that mean someone could modify its value while ur in the lock.
2. the lock should be on the min amount of code, what is the point of that check? What are the lock supposed to be locking exactly?

3. return without a unlock will bring all other threads to stop

4. with multiple threads there is a chance u got a reference counter that is negative which is mostly wrong

How about to check m_refCount > 0 before decrement?

Release () {
    m.lock();
    if (m_refCount > 0) {
        m_refCount--;
        if (m_refCount == 0) {
            free(this);
        }
    }
    m.unlock();
    return m_refCount;
}

I love this question. So under the nose and so real.