Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fixed CAN transport media TX callback lifetime. #437

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Conversation

serges147
Copy link
Collaborator

@serges147 serges147 commented Feb 6, 2025

Fix for issue #438:

  • Fixed callback lifetime bug in CAN transport; added unit test to specifically verify TX media callback lifetime.
  • Although there is no such issue at UDP transport, still added similar test for udp as well.

@serges147 serges147 self-assigned this Feb 6, 2025
// There was nothing successfully polled,
// AND won't be in the (near) future (b/c queue is empty),
// so we are done with this TX media - no more callbacks for now (until brand new TX transfer).
media.tx_callback().reset();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Such .reset was lost after recent refactoring (to use new canardTxPoll api).

// There is nothing to send anymore, so we are done with this media TX socket - no more callbacks for now.
// There was nothing successfully sent (otherwise we would have `return`-ed earlier),
// AND won't be in the (near) future (b/c queue is empty),
// so we are done with this TX media - no more callbacks for now (until brand new TX transfer).
media.txSocketState().callback.reset();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This important line now under unit tests - previously, you could comment it and unit tests would be still green. Not anymore.

Copy link
Contributor

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

❤️

@serges147 serges147 added this pull request to the merge queue Feb 7, 2025
Merged via the queue into main with commit a3be9d3 Feb 7, 2025
22 checks passed
@serges147 serges147 deleted the sshirokov/can_issue branch February 7, 2025 06:23
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