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

revert to index based notify loop #212

Merged

Conversation

TheCoconutChef
Copy link
Contributor

This fixes an issue in which calls to link occurring during notification would invalidate the iterators of the structured for loop. Calling link as a result of a reader::watch callback can occur quite naturally.

By using the size of the children vector at the point at which the for loop is initiated, and by using the [] operator in order to access the children within the vector, the issue is avoided.

This fixes an issue in which calls to link occurring during notification
would invalidate the iterators of the structured for loop. Calling link
as a result of a reader<T>::watch callback can occur quite naturally.

By using the size of the children vector at the point at which the for
loop is initiated, and by using the [] operator in order to access the
children within the vector, the issue is avoided.
Copy link
Owner

@arximboldi arximboldi left a comment

Choose a reason for hiding this comment

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

Oh I missed this during the review! good catch! It would be nice to have a test to catch this one in the future.

@TheCoconutChef
Copy link
Contributor Author

Oh I missed this during the review! good catch! It would be nice to have a test to catch this one in the future.

Agreed. I'm trying my hand at a test for this. There's something tricky about the fact that the incorrect version of the loop falls into UB.

@arximboldi arximboldi merged commit 93b5962 into arximboldi:master Sep 20, 2024
7 checks passed
@arximboldi
Copy link
Owner

Merging already, you can make a new PR for the test later if you feel like it...

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.

2 participants