fix segmentation fault when adding multiple executors simultaneously #1632
Conversation
There was a problem hiding this comment.
I am not sure if this is related to #1524, but on_shutdown_callbacks_ should be protected by on_shutdown_callbacks_mutex_.
| @@ -344,6 +344,7 @@ Context::shutdown(const std::string & reason) | |||
| rclcpp::Context::OnShutdownCallback | |||
| Context::on_shutdown(OnShutdownCallback callback) | |||
| { | |||
| std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_); | |||
There was a problem hiding this comment.
probably not only adding mutex here, but also all access to on_shutdown_callbacks_?
There was a problem hiding this comment.
I think this is not possible without changing the API as the reference is returned
There was a problem hiding this comment.
We should deprecate in the future those getters, returning a reference to an internal container isn't great.
I think that the mutex should also be taken here before calling all the callbacks:
{
std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
for (const auto & callback : on_shutdown_callbacks_) {
callback();
}
}
There was a problem hiding this comment.
We should deprecate in the future those getters, returning a reference to an internal container isn't great.
In the general case, I don't agree with this, but in this particular case, I agree.
We could return a copy (since they should be trivially copyable) and lock that operation.
Or we could just drop the accessors. I think I added them just because having hidden state like that can bite you later if you need access to it and have to add an API to get it, or work around it by keeping track of them outside this class (duplicating the effort). So I'd lean towards returning a copy and protecting that with the mutex.
|
@DensoADAS can you fix the commit so it passes our DCO check? |
|
I can help address the comments, but I cannot fix the DCO thing. |
|
@DensoADAS friendly ping, could you sign the commit so the DCO check passes? Thanks! |
|
Hi, just saw the mail. Do I have to sign off with a different mail/name? |
|
The DCO bot can be a bit picky at times. Right now it says this: I think it is unhappy because the author (Joshua Hampp j.hampp@denso-adas.de) doesn't match the sign-off line (Joshua Hampp j.hampp@eu.denso.com). |
…hich led to a race condition (seg fault) when creating multiple executors (multithreaded) Signed-off-by: Joshua Hampp <j.hampp@eu.denso.com> Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
DensoADAS
force-pushed
the
dnae_adas/bugfix_racecondition_in_context_creation
branch
from
6eb34a8
to
eb13e5f
Compare
Signed-off-by: Joshua Hampp <j.hampp@eu.denso.com> Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
DensoADAS
force-pushed
the
dnae_adas/bugfix_racecondition_in_context_creation
branch
from
61a48a2
to
ce7ded4
Compare
|
Superseded by #1639, which fixed another thing + this. |
|
Thanks @DensoADAS for the contribution! |
While creating multiple SingleThreadedExecturors in different thread I ran in crashes.
By adding a lock for "on_shutdown_callbacks_" in "on_shutdown" which is called in the constructor of SingleThreadedExecturors I solved the issue.