Check for nullptr when locking

[duke8253]

We need to check if the mutex pointer is actually pointing to something before using it, such that calling SCOPED_MUTEX_LOCK doesn't crash.

[masaori335]

I understand this change save the crash, but I'm not sure this is what we should do. Because this change allows us to write something like below.

Ptr<ProxyMutex> mutex = nullptr;
// long long line...
SCOPED_MUTEX_LOCK(lock, mutex, this_ethread());

If I look at the SCOPED_MUTEX_LOCK, I assume the mutex is acquired in this scope, but actually, it's not.

IMO, I think we should make sure nullptr is not given by release assert instead of nullptr check.

[SolidWallOfCode]

In that case, there are no unlocked mutexes when the continuation is called. There are cases where having no mutex is desirable, for instance all of the data is static and locking is only overhead. How should that be done?

[maskit]

There are cases where having no mutex is desirable

I think we should have another macro for the use cases. This is scoped mutex lock.

[zwoop]

What's particularly bothersome here is that the entire contract on how we use mutexes has changed now. I'm definitely not sure this is the right thing to do. And, this patch doesn't change any existing usage, so I imagine there are cases now where we double check the mutex variable. And, in a majority (almost always?) of the time, mutexes are not not nullptr, so we're doing this additional check for nothing.

[zwoop]

This feels like 1) It hides real issues 2) Confuses an existing, well known API, 3) Doesn't follow a contract the way it looks / was designed. I think we should revert this, and places where a mutex can be nullptr, it's definitely preferable to be explicit, e.g. I find this more obvious:

if (m) {
    SCOPED_MUTEX_LOCK(l, m);
}

whereas after this patch, SCOPED_MUTEX_LOCK can silently become a no-op, just as @masaori335 points out.

[SolidWallOfCode]

Yes, we're addressing all of that over on #5879. Our problem is, absent a change like this, we get assert crashes in high concurrency TLS operations.

[bryancall]

Cherry picked to the 9.0.x branch.