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

Ensure assert_valid_syn_id() is always called with correct thread id #3092

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

heplesser
Copy link
Contributor

Automated thread analysis revealed a data race because ModelManager::get_connection_model() calls assert_valid_syn_id() without thread argument

assert_valid_syn_id( syn_id );

thus using the default thread number 0. This can lead to data races during parallel construction of new models during CopyModel. This PR removes the default value for the thread so avoid data races.

@heplesser heplesser added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 2, 2024
Copy link
Contributor

@otcathatsya otcathatsya left a comment

Choose a reason for hiding this comment

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

Looks good; would it be an option to set the current thread id as the parameter default? From what I can see the method is only used inside connection and model manager who always call it with the same parameterization.

@heplesser
Copy link
Contributor Author

Looks good; would it be an option to set the current thread id as the parameter default? From what I can see the method is only used inside connection and model manager who always call it with the same parameterization.

The parameter default must be a constant, so we can't make the current thread the default.

@otcathatsya
Copy link
Contributor

otcathatsya commented Feb 12, 2024

Looks good; would it be an option to set the current thread id as the parameter default? From what I can see the method is only used inside connection and model manager who always call it with the same parameterization.

The parameter default must be a constant, so we can't make the current thread the default.

My bad; could do an overload to produce the same behaviour?

Copy link
Contributor

@suku248 suku248 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @heplesser

@suku248 suku248 merged commit 1c4569f into nest:master Feb 27, 2024
24 checks passed
@heplesser heplesser deleted the fix-missing-thread-arg branch April 24, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants