-
Notifications
You must be signed in to change notification settings - Fork 592
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
Fix DataStorm filter initialization #3277
Conversation
cpp/include/DataStorm/InternalT.h
Outdated
@@ -439,13 +439,18 @@ namespace DataStormI | |||
|
|||
template<typename FF> void init(const std::string& name, FF&& lambda) | |||
{ | |||
_name = name; | |||
_lambda = std::forward<FF>(lambda); | |||
std::unique_lock lock(_mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m thinking the mutex is probably not required, the factory is owned by the topic instance and the instances cannot be shared between threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not required, you should add a comment instead of a mutex. And clearly the other functions (getName etc.) don't use a mutex.
cpp/include/DataStorm/InternalT.h
Outdated
@@ -439,13 +439,18 @@ namespace DataStormI | |||
|
|||
template<typename FF> void init(const std::string& name, FF&& lambda) | |||
{ | |||
_name = name; | |||
_lambda = std::forward<FF>(lambda); | |||
std::unique_lock lock(_mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not required, you should add a comment instead of a mutex. And clearly the other functions (getName etc.) don't use a mutex.
_name = name; | ||
_lambda = std::forward<FF>(lambda); | ||
std::unique_lock lock(_mutex); | ||
if (!_lambda) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is init can be called multiple times? This deserves a comment since it's unexpected with a function named init
.
Fix #2926