Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() #603

ivanpauno · 2020-07-15T20:52:27Z

The other half of #601 to completely fix #588.

The assumption in comment

Lines 271 to 272 in be1deb6

    
           # We live in our own namespace and have created no other participants, so 
        
           # there can't be any of these events.

was wrong because some of the objects created in the previous test cases were still around.

ivanpauno · 2020-07-15T21:01:31Z

Linux (failure reported in Default incompatible qos callback should be set when there's no user specified callback #601 is gone, the other failure is unrelated an preexisting)

rclpy/test/test_qos_event.py

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-07-15T22:17:07Z

Linux

clalancette

Change seems reasonable to me, but the test failures seem odd. At least, I don't see test_waitable_with_timer failing in the nightly Linux. @ivanpauno any thoughts?

ivanpauno · 2020-07-16T12:50:58Z

Change seems reasonable to me, but the test failures seem odd. At least, I don't see test_waitable_with_timer failing in the nightly Linux. @ivanpauno any thoughts?

That test is extremely unrelated, so it's maybe flaky.

I'm testing here only rmw_cyclonedds which is not the usual nightly conditions (the reported bug #588 didn't happen in nightlies).
I cannot find a similar failure in the only cyclonedds job in buld.ros2.org, so I will investigate a bit more before merging.

Linux (including fastrtps)
Linux (only cyclone)

ivanpauno · 2020-07-16T12:51:19Z

@ros-pull-request-builder retest this please

ivanpauno · 2020-07-16T13:27:35Z

That test is extremely unrelated, so it's maybe flaky.

It seems to be related actually 😕

…ng them Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-07-16T14:22:53Z

Ok, I think I've got it right now:

Linux (only cyclonedds)

edit: Updated after last commit

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

clalancette · 2020-07-16T14:38:48Z

This is looking good. However, there is enough change in here that I'll suggest a more thorough test before merging. I think we should run CI on all platforms and test with --packages-above rclpy just to make sure there isn't something else lurking out there.

ivanpauno · 2020-07-16T15:28:46Z

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2020-07-16T19:03:51Z

@clalancette the failing windows build is due to a bug in rosbag (nightlies did fail too).
I think this is safe to merge now, it's highly unlikely that this behavior is platform dependent.

… publisher.destroy()/subscription.destroy() (ros2#603) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

… publisher.destroy()/subscription.destroy() (#603) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

… publisher.destroy()/subscription.destroy() (#603) (#625) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno added bug Something isn't working in review Waiting for review (Kanban column) labels Jul 15, 2020

ivanpauno requested a review from dirk-thomas July 15, 2020 20:52

ivanpauno self-assigned this Jul 15, 2020

ivanpauno mentioned this pull request Jul 15, 2020

Default incompatible qos callback should be set when there's no user specified callback #601

Merged

clalancette reviewed Jul 15, 2020

View reviewed changes

rclpy/test/test_qos_event.py Outdated Show resolved Hide resolved

ivanpauno added 3 commits July 15, 2020 21:51

Cleanup objects in test_qos_event between each test_case

c27aa82

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Change destruction ordering

5733185

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Fix the real issue

ef3b785

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/fix-test-qos-event branch from be1deb6 to ef3b785 Compare July 15, 2020 22:14

Delete unused import

ec877cc

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

dirk-thomas approved these changes Jul 15, 2020

View reviewed changes

clalancette reviewed Jul 16, 2020

View reviewed changes

Remove waitables created by publishers and subscription when destroyi…

5da98cb

…ng them Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno requested review from clalancette and dirk-thomas July 16, 2020 14:23

fix typo

9665d8e

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

clalancette approved these changes Jul 16, 2020

View reviewed changes

ivanpauno changed the title ~~Cleanup objects in test_qos_event between each test_case~~ Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() Jul 16, 2020

ivanpauno merged commit 75c51c9 into master Jul 16, 2020

delete-merged-branch bot deleted the ivanpauno/fix-test-qos-event branch July 16, 2020 20:42

ivanpauno added this to Proposed in Foxy Patch Release 2 via automation Jul 16, 2020

hidmic mentioned this pull request Jul 16, 2020

No callback when AUTOMATIC liveliness is lost ros2/rmw_fastrtps#384

Closed

jacobperron removed this from Proposed in Foxy Patch Release 2 Aug 5, 2020

jacobperron added this to Proposed in Foxy Patch Release 3 via automation Aug 5, 2020

jacobperron moved this from Proposed to To do in Foxy Patch Release 3 Aug 5, 2020

jacobperron moved this from To do to Needs backport in Foxy Patch Release 3 Aug 5, 2020

jacobperron pushed a commit that referenced this pull request Oct 1, 2020

Destroy event handlers owned by publishers/subscriptions when calling…

116a112

… publisher.destroy()/subscription.destroy() (#603) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

jacobperron mentioned this pull request Oct 1, 2020

[foxy] Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() (#603) #625

Merged

jacobperron removed this from Needs backport in Foxy Patch Release 3 Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() #603

Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() #603

ivanpauno commented Jul 15, 2020

ivanpauno commented Jul 15, 2020 •

edited

ivanpauno commented Jul 15, 2020

clalancette left a comment

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020 •

edited

clalancette commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

	# We live in our own namespace and have created no other participants, so
	# there can't be any of these events.

Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() #603

Destroy event handlers owned by publishers/subscriptions when calling publisher.destroy()/subscription.destroy() #603

Conversation

ivanpauno commented Jul 15, 2020

ivanpauno commented Jul 15, 2020 • edited

ivanpauno commented Jul 15, 2020

clalancette left a comment

Choose a reason for hiding this comment

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020 • edited

clalancette commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 16, 2020

ivanpauno commented Jul 15, 2020 •

edited

ivanpauno commented Jul 16, 2020 •

edited