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

Fix concurrency issues #1256

Merged
merged 3 commits into from Dec 27, 2023
Merged

Conversation

fbrouille
Copy link
Contributor

This PR aims to fix the concurrency issues in code generated by the macro helper attributes object_subclass_dynamic and object_interface_dynamic

…amic'

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
/// Do nothing as the object interface has been registered on implementation load.
#[inline]
fn register_interface() -> #crate_ident::Type {
unsafe { <#crate_ident::Type as #crate_ident::translate::FromGlib<#crate_ident::ffi::GType>>::from_glib(*Self::get_type_mut()) }
let gtype = #gtype_status.load(::std::sync::atomic::Ordering::Relaxed);
Copy link
Member

Choose a reason for hiding this comment

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

Is this somewhere protected also via a mutex? Otherwise it would probably be safer to use SeqCst here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No mutex here, I thought atomic was enough. But yes Relaxed is a weak memory ordering and other threads might not see the change 'immediately' (however this is not really a problem because a type can be registered several times and Glib system always return the same GType value). For synchronization purpose, maybe SeqCst is too strong. I think Acquire and Release are enough here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, Acquire / Release seems to be enough here

Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Otherwise seems good to me

…namic'

Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
Signed-off-by: fbrouille <fbrouille@users.noreply.github.com>
@sdroege sdroege merged commit df9d809 into gtk-rs:master Dec 27, 2023
46 of 48 checks passed
@fbrouille fbrouille deleted the fix_concurrency_issues branch December 27, 2023 20:45
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.

None yet

2 participants