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

mainThreadDocuments.tryCreateDocument() is bypassing our model reference story #21709

Closed
bpasero opened this issue Mar 2, 2017 · 5 comments
Closed
Assignees
Labels
debt Code quality issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Mar 2, 2017

Extensions can create documents in our system that eventually cause text models to be created (via $tryOpenDocument).

We correctly use createModelReference so that model lifecycle is ref-counted, however we fail to dispose this reference at any time (as outlined by this comment:

// TODO@Joao TODO@Joh when should this model reference be disposed?
// ref.dispose();

This blocks me from adopting references for untitled/files (TextFileEditorModelManager disposes models it doesn't own) because unless I can rely on people disposing these references properly, we would end up with major leaks of models throughout the system (this is the reason why today I have weird code that disposes file models if I think they are no longer being used).

Setting to March so that we can finish this story that we started.

Also slightly related: we need a way to signal that a resource is literally gone. So, no matter how many references have been created, a way to force disposal of the underlying model (e.g. when a file gets deleted). I guess we will need this for extensions too (e.g. git).

@jrieken
Copy link
Member

jrieken commented Mar 6, 2017

Not really sure how that blocks @bpasero because he should only release the models he has created, not others which is what #17888 is about. The idea with extensions is that they shouldn't be able to hold on to models indefinitely and thereby kill us. For the extension side we will likely empty some timeout logic once a document isn't visible anymore.

@bpasero
Copy link
Member Author

bpasero commented Mar 6, 2017

@jrieken it blocks because when I no longer dispose models "I do not own", we start to leak any file model that is created from the extension host side because today I see no code that disposes a file model reference on the extension host. That is the essence of what this bug is about.

@jrieken
Copy link
Member

jrieken commented Mar 6, 2017

I am not saying this isn't a valid bug and I do appreciate that you try to fix leaks but one central place isn't cutting it because things like preview-html and embedded editor die because of that. The issue of releasing documents created by extension is easy, but I just wanna make sure the workbench stops disposing models it doesn't own

@jrieken jrieken closed this as completed in 3a5f621 Mar 6, 2017
@jrieken
Copy link
Member

jrieken commented Mar 6, 2017

I have restored the 3 min rule. Now we have to start to return reference that are mortal.

@bpasero
Copy link
Member Author

bpasero commented Mar 6, 2017

Thanks 👍

jrieken added a commit that referenced this issue Mar 22, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

3 participants