Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix #862 - unsub of a message ID that is already unsubbed #865

Merged

Conversation

CDKnightNASA
Copy link
Contributor

Closes #862

Describe the contribution
After the changes implemented in #101, there may be routing table entries with no subscribers (RoutePtr->ListHeadPtr would be NULL.) This could cause a seg-fault. Also, even if there are entries in the routing table, there will be no event generated if the unsubscribe does not find a matching route entry.

Testing performed
Ran unit test (with updated event count.)

Expected behavior changes
Repeated unsubscriptions should function fine, generating an informational event if there is not a current subscription.

System(s) tested on
Debian 10.5

Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov

@CDKnightNASA CDKnightNASA requested a review from skliper September 4, 2020 15:58
@CDKnightNASA CDKnightNASA self-assigned this Sep 4, 2020
@skliper
Copy link
Contributor

skliper commented Sep 4, 2020

I appreciate that in the last couple of weeks we've seen three different loop structures to accomplish the same logic. While loop with an incrementer, for loop with a break, and now a for loop with dual exit condition. Way to keep things interesting!

@astrogeco
Copy link
Contributor

I appreciate that in the last couple of weeks we've seen three different loop structures to accomplish the same logic. While loop with an incrementer, for loop with a break, and now a for loop with dual exit condition. Way to keep things interesting!

I don't know if you're genuinely excited or not-so-subtly indicating that we should choose one way and stick to it as much as possible :)

@CDKnightNASA
Copy link
Contributor Author

I feel like we keep going around and around on this...

(Sorry, I just couldn't resist!)

@yammajamma
Copy link
Contributor

@CDKnightNASA is this ready for a CCB review to get more inputs?

@CDKnightNASA CDKnightNASA added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Sep 8, 2020
@CDKnightNASA
Copy link
Contributor Author

@CDKnightNASA is this ready for a CCB review to get more inputs?

yes thx

@astrogeco
Copy link
Contributor

CCB 2020-08-09 APPROVED, How do we ensure we test and catch this? Open a new issue for creating a test.

@skliper
Copy link
Contributor

skliper commented Sep 9, 2020

I opened #873 for the added test case.

@skliper
Copy link
Contributor

skliper commented Sep 10, 2020

Added the test case in #875

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it passes test w/ #875 I'm good with this.

@yammajamma yammajamma marked this pull request as ready for review September 10, 2020 16:13
@yammajamma yammajamma changed the base branch from main to integration-candidate September 10, 2020 17:18
@yammajamma yammajamma added IC-20200909 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 10, 2020
@yammajamma yammajamma merged commit fd85c70 into nasa:integration-candidate Sep 10, 2020
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If a message is subscribed, then unsubscribed, additional unsubscribes do not raise error events
4 participants