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

[dbconnector] protect db information with mutex #423

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

yxieca
Copy link
Contributor

@yxieca yxieca commented Dec 2, 2020

STL containers are only thread safe for reads. But not for writes. protect the db information variables with rwlock.

Signed-off-by: Ying Xie [email protected]

STL containers are only thread safe for reads. But not for writes.
protect the db information variables with rwlock.

Signed-off-by: Ying Xie <[email protected]>
@yxieca
Copy link
Contributor Author

yxieca commented Dec 2, 2020

This change is intended to address issue sonic-net/sonic-buildimage#6103.

However, after added this tentative protection, I realized that there is no code removing anything from m_db_info. So I guess this protection won't really protect us against the issue above?

@@ -54,6 +55,7 @@ class SonicDBConfig
static bool isGlobalInit() { return m_global_init; };

private:
static pthread_rwlock_t db_info_lock;
Copy link
Contributor

Choose a reason for hiding this comment

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

make this static seems like bad idea to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the information under protection are static variables. That is why this lock is also static.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, entire class is static, which is bad XD

@kcudnik
Copy link
Contributor

kcudnik commented Dec 2, 2020

@yxieca What is main motivation for this change? You are manually locking/unlocking lock in many places and it could be easy to forget to do that when code will grow. At least wrapping that to class and release in destructor. Also is std::shared_lock what you are looking for?
I would be against using rwlock lock here, unless you really know what you are doing and you know what your flow/data is. I'm assuming that you are worrying that some of the threads will be starved to death? Do you have any data on which you are basing this proposal?

also mistake in title "dbconnectgor"

@yxieca yxieca changed the title [dbconnectgor] protect db information with rwlock [dbconnector] protect db information with rwlock Dec 2, 2020
@kcudnik
Copy link
Contributor

kcudnik commented Dec 2, 2020

Is issue related to m_inst_info/m_db_info ? those are static map, and in general it's advised to not uses statics, since they "wired" things can happen process is shutdown, depends on complexity, and threads running, it could happen that some destroyed data is still in use which can lead to some really hard to track errors

@kcudnik
Copy link
Contributor

kcudnik commented Dec 2, 2020

This SonicDBConfig class don't see to me as critical performance class, so std::recursive_mutex would make more sense and more clean solution.

Also, i know not the scope of this PR, but i don't see a reason to make this normal non static class, not even a singleton.

@lguohan lguohan requested a review from qiluo-msft December 3, 2020 18:39
@qiluo-msft
Copy link
Contributor

void SonicDBConfig::initializeGlobalConfig(const string &file)

All the initialize... functions should be run in single thread environment. Then all other functions are read-only.
Seems an application usage bug if the assumption is broken. Why application use multiple threads for any write operation?


Refers to: common/dbconnector.cpp:70 in 74cf24e. [](commit_id = 74cf24e, deletion_comment = False)

@@ -141,11 +142,13 @@ void SonicDBConfig::initializeGlobalConfig(const string &file)
catch (domain_error& e)
{
SWSS_LOG_ERROR("key doesn't exist in json object, NULL value has no iterator >> %s\n", e.what());
pthread_rwlock_unlock(&db_info_lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

pthread_rwlock_unlock [](start = 12, length = 21)

Better to use std::lock_guard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. If we decide to move forward. I'll make change accordingly

Copy link
Contributor

@kcudnik kcudnik Dec 9, 2020

Choose a reason for hiding this comment

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

yea, and use recursive_mutex as discussed

@@ -6,6 +6,8 @@
#include <unordered_map>
#include <utility>
#include <memory>
#include <pthread.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

you dont need probably this include

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. I added that earlier. it is no longer needed now.

kcudnik
kcudnik previously approved these changes Jan 8, 2021
@yxieca yxieca changed the title [dbconnector] protect db information with rwlock [dbconnector] protect db information with mutex Jan 12, 2021
@yxieca yxieca marked this pull request as ready for review January 12, 2021 23:50
@yxieca yxieca merged commit fc64e3a into sonic-net:master Jan 12, 2021
@yxieca yxieca deleted the dbconnector branch January 12, 2021 23:52
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.

3 participants