[logger] Make map access thread safe and proper terminate thread #510
Conversation
Signed-off-by: kcudnik <[email protected]>
|
test error on test_ConfigDBSubscribe not related to PR |
|
@arlakshm can you check this test_ConfigDBSubscribe ? |
common/logger.cpp
Outdated
|
|
||
| m_settingThread = nullptr; | ||
|
|
||
| m_runSettingThread = true; |
There was a problem hiding this comment.
Might consider putting m_runSettingThread = true; in restartSettingThread? Not sure which is cleaner.
common/logger.cpp
Outdated
|
|
||
| m_settingChangeObservers.get(key).first(key, value); |
| linkToDb(dbName, swssPrioNotify, defPrio); | ||
| logger.m_settingThread.reset(new std::thread(&Logger::settingThread, &logger)); | ||
|
|
||
| getInstance().restartSettingThread(); |
There was a problem hiding this comment.
Is restarting the thread actually required if the thread is already started and the data structures are thread-safe? Put another way: is there any reason to maintain a separate linkToDbNative and linkToDb vs consolidating them into one function which starts the thread if needed?
There was a problem hiding this comment.
thats a good question, im just fixing this instance here, since previously if you call linkToDbNative twice, it will crash, and whether we should have only 1 thread, not sure about that, this PR is just to make those maps thread safe + plus crahs bug fix not changing behavior, @qiluo-msft can you have opinion?
|
ping |
common/concurentmap.h
Outdated
| namespace swss | ||
| { | ||
| template <typename K, typename V> | ||
| class ConcurentMap |
|
ping |
1 similar comment
|
ping |
Signed-off-by: kcudnik [email protected]
Fixes sonic-net/sonic-buildimage#8177
I also made some minor code style refactoring