Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support SharedMutex. #1975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

support SharedMutex. #1975

wants to merge 1 commit into from

Conversation

liyichao
Copy link

@liyichao liyichao commented Oct 30, 2022

support SharedMutex that is compatible with c++17 std::shared_mutex. This patch uses existing class Mutex, and is migrated from golang, see https://github.com/golang/go/blob/master/src/sync/rwmutex.go

@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 31, 2022

related: #1031

@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 31, 2022

CI is failed

@liyichao
Copy link
Author

liyichao commented Dec 1, 2022

@wwbmmm I have fixed the build, the CI failure seems unrelated to this patch, please help check it.

@wwbmmm
Copy link
Contributor

wwbmmm commented Dec 1, 2022

@guodongxiaren Please help to see this CI failure, the error is so strange.

@guodongxiaren
Copy link
Member

guodongxiaren commented Dec 1, 2022

I have fixed this issue in #2030 @wwbmmm please review and merge in master.

@wwbmmm
Copy link
Contributor

wwbmmm commented Dec 2, 2022

@wwbmmm I have fixed the build, the CI failure seems unrelated to this patch, please help check it.

@liyichao the master branch has fixed the CI issue, please merge master and retry

@liyichao
Copy link
Author

liyichao commented Dec 2, 2022

@wwbmmm I have merged master, the workflows need approval

src/bthread/shared_mutex.h Show resolved Hide resolved
src/bthread/shared_mutex.cpp Show resolved Hide resolved
src/bthread/shared_mutex.cpp Show resolved Hide resolved
src/bthread/shared_mutex.cpp Outdated Show resolved Hide resolved
src/bthread/shared_mutex.cpp Show resolved Hide resolved
src/bthread/shared_mutex.cpp Outdated Show resolved Hide resolved
src/bthread/shared_mutex.cpp Outdated Show resolved Hide resolved
src/bthread/shared_mutex.cpp Show resolved Hide resolved
src/bthread/shared_mutex.cpp Outdated Show resolved Hide resolved

static constexpr int32_t max_readers = 1 << 30;
Mutex _w;
uint32_t* _writer_butex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改成butil::atomic<uint32_t>*,保证操作的内存序

static constexpr int32_t max_readers = 1 << 30;
Mutex _w;
uint32_t* _writer_butex;
uint32_t* _reader_butex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

@wwbmmm
Copy link
Contributor

wwbmmm commented Dec 6, 2022

建议补充单测

Copy link
Member

@chenzhangyi chenzhangyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要补充单测和注释,特别是牵涉到atomic的部分。 要注释里面说清楚为什么这种实现在所有组合下都是线程安全的。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants