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 Tf_RegistryManagerImpl singleton instance not being deleted #3133

Closed
wants to merge 1 commit into from

Conversation

moshev
Copy link

@moshev moshev commented Jun 20, 2024

Description of Change(s)

Delete the dynamically allocated Tf_RegistryManagerImpl singleton instance when the static TfRegistryManager singleton instance which uses it is destroyed.

Fixes Issue(s)

  • 3088
  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-9782

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@meshula
Copy link
Member

meshula commented Jun 21, 2024

Hi @moshev ! I think you meant to make this PR against the dev branch, not the release branch. That is why there are a couple of hundred extra commits in it :) Could you try to recreate it against dev please?

@moshev
Copy link
Author

moshev commented Jun 21, 2024

Hi @moshev ! I think you meant to make this PR against the dev branch, not the release branch. That is why there are a couple of hundred extra commits in it :) Could you try to recreate it against dev please?

Oops. Yes, of course.

@moshev moshev changed the base branch from release to dev June 21, 2024 06:19
@moshev
Copy link
Author

moshev commented Jun 21, 2024

I changed the base to dev branch in this review, let me know if there's still an issue.

@meshula
Copy link
Member

meshula commented Jun 21, 2024

Much better :) Thanks!

@jesschimein
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgovil dgovil added the needs review Issue needing input/review by the repo maintainer (Pixar) label Oct 31, 2024
@spiffmon
Copy link
Member

spiffmon commented Nov 1, 2024

Hi @moshev , we don't think we can accept this PR. Nothing in OpenUSD currently deletes the singleton, and it would not be safe, generally, to do so, in our codebase. Of particular concern is static destruction ordering issues, as there are many other destructors that may access the registry.

@spiffmon spiffmon closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Issue needing input/review by the repo maintainer (Pixar)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants