*: Introduce MutexProtected and SharedMutexProtected

Lloyd-Pottiger commented 4 months ago

What problem does this PR solve?

Issue Number: ref #6233

Problem Summary:

What is changed and how it works?

Introduce MutexProtected and SharedMutexProtected

In C++, there is a RAII class for automatic locking & unlocking:

{
    MutexLocker locker(thing->mutex);
    use(thing->field);
}

The MutexLocker locks the mutex when constructed and unlocks it when destroyed. No need for a manual call to mutex_unlock() anymore.

That’s already pretty good! However, the MutexLocker approach still has some major shortcomings:

You can still forget to lock the mutex and access field anyway.
Developers who are unfamiliar with this code may not realize that mutex and field have this important relationship.

So we introduce MutexProtected and SharedMutexProtected, which makes it significantly easier to use mutexes correctly

struct Thing {
    MutexProtected<Field> field;
};

thing->field.with([&](Field& field) {
    use(field);
});

Essentially, MutexProtected is a bundled mutex and T. However, you can’t access the T directly! The only way we’ll let you access the T is by calling with() and passing it a callback that takes a T& parameter.

When called, with() locks the mutex, then invokes the callback, and finally unlocks the mutex again before returning.

As you can see, we’ve now also solved the issue of someone forgetting to lock the mutex before accessing the field. And not only that, but since the mutex and field have been combined into a single variable, you no longer have to be aware of the relationship between the two. It’s been encoded into the type system!

Reference: https://awesomekling.github.io/MutexProtected-A-C++-Pattern-for-Easier-Concurrency/

Check List

Tests

[ ] Unit test
[ ] Integration test
[ ] Manual test (add detailed scripts or steps below)
[ ] No code

Side effects

[ ] Performance regression: Consumes more CPU
[ ] Performance regression: Consumes more Memory
[ ] Breaking backward compatibility

Documentation

[ ] Affects user behaviors
[ ] Contains syntax changes
[ ] Contains variable changes
[ ] Contains experimental features
[ ] Changes MySQL compatibility

Release note

None

CalvinNeo commented 4 months ago

There are MutexLockWrap and SharedMutexLockWrap already. IMO, the new class here is also to make acquiring the lock the only way to access a field. It provides the with function though. So, can we unified them? It is confusing if we have two versions of utils which usages are similar.

Lloyd-Pottiger commented 3 months ago

There are MutexLockWrap and SharedMutexLockWrap already. IMO, the new class here is also to make acquiring the lock the only way to access a field. It provides the with function though. So, can we unified them? It is confusing if we have two versions of utils which usages are similar.

MutexLockWrap and SharedMutexLockWrap seems only contain a mutex and provide some function to acquire a lock. By inherit one of them, we can get the mutex and these functions.

MutexProtected and SharedMutexLockWrap are mainly to warp a field and its mutex, and we do not need to worry about whether there are some places that access the field but forget to acquire a lock.

CalvinNeo commented 3 months ago

There are MutexLockWrap and SharedMutexLockWrap already. IMO, the new class here is also to make acquiring the lock the only way to access a field. It provides the with function though. So, can we unified them? It is confusing if we have two versions of utils which usages are similar.

MutexLockWrap and SharedMutexLockWrap seems only contain a mutex and provide some function to acquire a lock. By inherit one of them, we can get the mutex and these functions.

MutexProtected and SharedMutexLockWrap are mainly to warp a field and its mutex, and we do not need to worry about whether there are some places that access the field but forget to acquire a lock.

I think they both aimed to provide some way to ensure a lock-raii sematics, which means a resource could only be accessed by acquiring a lock, and the lock will be released automaticlly when the resource is no longer accessed.

The difference is the XXXProtected methods are more elaborate.

Lloyd-Pottiger commented 3 months ago

I don't get that. I find operator-> can access value in Locked. So I think the Locked should not outlive value which it is protected.

@CalvinNeo Lock can access value, so Lock can not be moved, otherwise the Locke maybe outlive value.

CalvinNeo commented 3 months ago

value

IMO, Lock can still outlive value, even if it is pinned

Lloyd-Pottiger commented 3 months ago

value

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

CalvinNeo commented 3 months ago

value

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

Consider the value referenced is not a auto-scoped variable on stack, but allocated on heap and managed by some std::map or std::vector. Then I can destroy the value in another thread.

Lloyd-Pottiger commented 3 months ago

value

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

Consider the value referenced is not a auto-scoped variable on stack, but allocated on heap and managed by some std::map or std::vector. Then I can destroy the value in another thread.

The provided constructor does not allow this.

CalvinNeo commented 3 months ago

value

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

Consider the value referenced is not a auto-scoped variable on stack, but allocated on heap and managed by some std::map or std::vector. Then I can destroy the value in another thread.

The provided constructor does not allow this.

I don't quite understand

Lloyd-Pottiger commented 3 months ago

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

Consider the value referenced is not a auto-scoped variable on stack, but allocated on heap and managed by some std::map or std::vector. Then I can destroy the value in another thread.

The provided constructor does not allow this.

I don't quite understand

IMO, your example can not be implemented basing on this PR. Or could you please show the code?

CalvinNeo commented 3 months ago

IMO, Lock can still outlive value, even if it is pinned

Could you please give an example?

Consider the value referenced is not a auto-scoped variable on stack, but allocated on heap and managed by some std::map or std::vector. Then I can destroy the value in another thread.

The provided constructor does not allow this.

I don't quite understand

IMO, your example can not be implemented basing on this PR. Or could you please show the code?

OK, I found that the lockConst method is private, so the only way to access the protected variable is through with.

ti-chi-bot[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, JaySon-Huang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/pingcap/tiflash/blob/master/OWNERS)~~ [CalvinNeo,JaySon-Huang] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

ti-chi-bot[bot] commented 2 months ago

[LGTM Timeline notifier]

Timeline:

2024-06-12 07:16:47.56727454 +0000 UTC m=+535361.620586461: :ballot_box_with_check: agreed by CalvinNeo.
2024-07-16 06:43:54.730182791 +0000 UTC m=+336256.721124264: :ballot_box_with_check: agreed by JaySon-Huang.

pingcap / tiflash