Skip to content

Don't call destroy_node while spinning#674

Merged
clalancette merged 1 commit intomasterfrom
cottsay/destroy_node_order
Feb 8, 2021
Merged

Don't call destroy_node while spinning#674
clalancette merged 1 commit intomasterfrom
cottsay/destroy_node_order

Conversation

@cottsay
Copy link
Copy Markdown
Member

@cottsay cottsay commented Jan 28, 2021

This causes a segfault under CycloneDDS, and I don't see an easy way to prevent it from happening. Until we get to the bottom of that, just defer the call to destroy_node until after we know the spin thread has finished.

#663 tracks the issue that this change avoids.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

This causes a segfault under CycloneDDS, and I don't see an easy way to
prevent it from happening. Until we get to the bottom of that, just
defer the call to destroy_node until after we know the spin thread has
finished.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay requested a review from sloretz January 28, 2021 02:06
@cottsay cottsay self-assigned this Jan 28, 2021
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jan 28, 2021

Windows CI failed to install Qt :( - edit: which was probably fixed by ros2/ci#540

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jan 28, 2021

Windows rebuild: Build Status

@cottsay
Copy link
Copy Markdown
Member Author

cottsay commented Jan 28, 2021

Thanks, @sloretz. Looks like the rebuild came back green.

@clalancette
Copy link
Copy Markdown
Contributor

This probably "fixes" #666 , though it mostly works around it. I'm going to go ahead and merge this PR and close out #666; further fixes can be tracked by #663.

@clalancette clalancette merged commit b5e27a0 into master Feb 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the cottsay/destroy_node_order branch February 8, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants