Executor strong reference fix #2745
Conversation
|
Pulls: #2745
|
| if(ret) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
| if(ret) { | |
| break; | |
| } | |
| if (ret) { | |
| break; | |
| } |
| if(ret) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
| if(ret) { | |
| break; | |
| } | |
| if (ret) { | |
| break; | |
| } |
| if(ret) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
| if(ret) { | |
| break; | |
| } | |
| if (ret) { | |
| break; | |
| } |
|
|
||
| // wait for both subs to be connected | ||
| auto max_end_time = std::chrono::steady_clock::now() + std::chrono::milliseconds(1500); | ||
| while((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) { |
There was a problem hiding this comment.
| while((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) { | |
| while ((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) { |
| // publish some messages, until one subscriber fired. As both subscribers are | ||
| // connected to the same topic, they should fire in the same wait. | ||
| max_end_time = std::chrono::steady_clock::now() + std::chrono::milliseconds(100); | ||
| while(!sub1_works && !sub2_works) { |
There was a problem hiding this comment.
| while(!sub1_works && !sub2_works) { | |
| while (!sub1_works && !sub2_works) { |
There was a problem hiding this comment.
lgtm with style fixups
But, I do not believe it fixes #2678, it simply makes it less likely (at least for the multi-threaded executor case or two separate executor case). If async .reset() happens after the executor has acquired the lock, but before it starts executing the callback then you'll still potentially get into a callback after the "owning" class was reset, thus you still need to capture a weak_ptr in the callback.
The right answer to #2678 is documentation, imo.
This pr is still worth doing because it frees up resources more quickly, but it doesn't solve the original problem completely.
|
@marcoag FYI we want to get this backported to Jazzy and it does end up with an ABI break. I think it's important enough to get in, though. Let's coordinate that this gets in before the next sync. |
ros2#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. Signed-off-by: Janosch Machowinski <[email protected]>
This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <[email protected]>
jmachowinski
force-pushed
the
executor_strong_reference_fix
branch
from
c8bdffc to
c553699
Compare
|
Pulls: #2745
|
|
https://github.com/Mergifyio backport jazzy |
✅ Backports have been created
|
#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. * fix(Executor): Don't hold strong references to entities during spin This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <[email protected]> Co-authored-by: Janosch Machowinski <[email protected]> (cherry picked from commit 9c493c4)
#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. * fix(Executor): Don't hold strong references to entities during spin This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <[email protected]> Co-authored-by: Janosch Machowinski <[email protected]> (cherry picked from commit 9c493c4) Co-authored-by: Janosch Machowinski <[email protected]>
fixes #2678
@mjcarroll we should merge this ASAP and backport to jazzy, to keep the ABI breakage to one release