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

ReplaceService: directly disposing IModel should be avoided #21756

Closed
bpasero opened this issue Mar 2, 2017 · 2 comments
Closed

ReplaceService: directly disposing IModel should be avoided #21756

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

Comments

@bpasero
Copy link
Member

bpasero commented Mar 2, 2017

I am hunting through our code to find areas where IModel is being disposed directly without going through our reference counted world. Besides file/untitled land that are known to not have this adopted, the only other place I found was in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/search/browser/replaceService.ts#L80

@sandy081 you are directly disposing IModel, basically bypassing our reference counting lifecycle model. Is there any reason? This is ugly because it means in my world I also have to listen to IModel getting disposed to dispose my models.

I am unsure why you actually need to first resolve a file reference and then create a new model, could you not just use the same model for the left hand side of the diff editor?

fyi @joaomoreno

@bpasero bpasero added the debt Code quality issues label Mar 2, 2017
@sandy081
Copy link
Member

sandy081 commented Mar 2, 2017

First of all I am disposing the model by myself because I create the model and once model is no longer valid it is disposed even though the references are alive. Use case was that when file matches were cleared all corresponding replace preview models should be disposed. Since there wa no proper disposing strategy for model as mentioned here.

I am not sure if there is a way to pass same URIs for LHS and RHS while opening the editor and show different contents.

@bpasero
Copy link
Member Author

bpasero commented Mar 2, 2017

Good point about the model, somehow I thought you would dispose the model that was created via the reference. It is still a big ugly because the model you create ends up being send to the outside into my ResourceEditorModel which then has to listen to dispose on IModel to dispose itself, which in turn at some point will just close the editor because the input gets disposed...

I have not better idea right now what to do, closing.

@bpasero bpasero closed this as completed Mar 2, 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

2 participants