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

Constructors in eigen_typekit are not thread-safe #34

Closed
meyerj opened this issue Jul 1, 2019 · 4 comments
Closed

Constructors in eigen_typekit are not thread-safe #34

meyerj opened this issue Jul 1, 2019 · 4 comments
Labels

Comments

@meyerj
Copy link
Member

meyerj commented Jul 1, 2019

Currently constructors in package eigen_typekit have a local instance of the respective type which is initialized and returned by reference from the implemented operator(). This is not thread-safe at all because constructors are singletons registered once when the typekit is loaded.

Each call to operator() should therefore return a new instance by value.

@meyerj meyerj added the bug label Jul 1, 2019
ahoarau pushed a commit to ahoarau/rtt_geometry that referenced this issue Jul 5, 2019
@ahoarau
Copy link
Contributor

ahoarau commented Jul 5, 2019

Addressed in b8e97bc

@meyerj
Copy link
Member Author

meyerj commented Jul 22, 2019

Thanks for the patch!

Some functors still have mutable ptr member variables with a single instance that gets overwritten and is returned by value for each invocation (and in some cases the ptr instance is unused now). That is better than to return by const reference, but not thread-safe if the constructors are invoked concurrently from different threads. I suggest to eliminate all member variables and only return new instances, either a temporary or an instance created as a local variable and returned by value.

ahoarau pushed a commit to ahoarau/rtt_geometry that referenced this issue Jul 22, 2019
@ahoarau
Copy link
Contributor

ahoarau commented Jul 23, 2019

I added the patch. Sorry I got confused by who was holding the data. Should be good for merge now

@meyerj
Copy link
Member Author

meyerj commented Jul 23, 2019

Fixed in #26. Thanks to @ahoarau!

@meyerj meyerj closed this as completed Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants