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

output: store a weak ref in the global to avoid... #1589

Closed
wants to merge 1 commit into from

Conversation

cmeissl
Copy link
Collaborator

@cmeissl cmeissl commented Nov 16, 2024

...a cycle keeping the output alive indefinitely

Should get rid of the last cycle in smithay as mentionen in #1585 and hopefully fix #1584

...a cycle keeping the output alive indefinitely
@cmeissl
Copy link
Collaborator Author

cmeissl commented Nov 16, 2024

@ids1024 With this the check seems to work as expected in anvil (not saying we shouldn't think about an alternative)

@cmeissl cmeissl marked this pull request as ready for review November 16, 2024 20:31
@ids1024
Copy link
Member

ids1024 commented Nov 18, 2024

Ah. So when does global user data like this actually get freed? Only once every client that has had a registry with the global present has disconnected?


let Some(output) = global_data.output.upgrade() else {
tracing::warn!("trying to bind a destroyed output, forgot to disable a output global?");
return;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should and can just return here. Any request to the output would otherwise trigger a panic, given we didn't initialize it's data.

The warning is fine (and a good idea), but I think we should just initialize the OutputUserData with Weak::new.

@cmeissl
Copy link
Collaborator Author

cmeissl commented Nov 18, 2024

Ah. So when does global user data like this actually get freed? Only once every client that has had a registry with the global present has disconnected?

The global data itself is freed on remove_global. I actually started this before pulling in your changes, so looking at it again, the change here is unnecessary. Just tried it out and the primary_scanout_output in PrimaryScanoutOutput::update_from_render_element_states seems to work fine in anvil on master.

@cmeissl cmeissl closed this Nov 18, 2024
@ids1024
Copy link
Member

ids1024 commented Nov 18, 2024

Yeah, as long as this is freed immediately when remove_global is called, it makes a lot of sense for the global to hold a strong reference.

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.

update_from_render_element_states relies on strong_count to determine if Output is still valid
3 participants