-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Prevent LegacyGlobalOptionService from GC rooting PreviewWorkspace #67142
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be because we're not disposing the PreviewWorkspace which would have done the unsubscribe. Although this may be a good defense in depth, can we fix that too?
Or alternatively: if we're making this weak, can we just get rid of the Unregister method entirely? |
➡️ Ideally yes. In practice, the cancellation paths involved in ensuring that happens are extra non-trivial. I have no objection to trying to fix that, but it's lower priority and if we decide to service 17.5 with a fix for this issue we aren't going to want to bring both.
➡️ It probably wouldn't end up building up too large a collection of workspaces in the list to notify, but there are other concerns. Currently, notifications sent to a forgotten workspace will not be in the disposed state, but if we omit |
@sharwell: my concern isn't the unregister list specifically, but there's a lot of other stuff also happening in Workspace.Dispose() that we're also skipping, and I have no idea if any of that is safe to skip or whether that's also causing additional rooting. Basically, we're violating the contract of the Workspace, we got caught, and this isn't fixing that. If we're going to take this approach due to us servicing this for 17.5, then I'd request a few additional things:
I actually don't really like the idea that Workspace is Disposable so I don't mind the direction this PR takes, but this PR as written leaves us in a place that's likely to have regressions without a test, and we should fix that. |
I'm not sure this matters in practice. If someone is thinking about adding an unsubscribe to that method, it seems unlikely that a comment would have any impact on the outcome. We also have numerous other cases where we can accidentally keep memory alive due to event subscriptions. |
Even if the tests would catch them I can still imagine people asking "so are workspaces supposed to be disposed or not?" and there's not a clear answer either way. |
I'm going to get this merged since it fixes the one known issue. Hopefully in the future we can fix other cases we find where solutions are not disposed, but at least we won't be leaking memory from them. |
Fixes AB#1756080