Dangling text file models of deleted files hanging around in memory #98057

testforstephen · 2020-05-18T05:50:22Z

VSCode Version: 1.45.1
OS Version: Windows 10

Steps to Reproduce:
Screencast:

Open sample node project below in VS Code.
dnd.zip
Set the setting "typescript.updateImportsOnFileMove.enabled": "always" or "never".
Click to open multiple files (e.g. src/utils/ma.ts, src/utils/mb.ts, src/utils/mc.ts, src/utils/md.ts) in VS Code.
In File Explorer, drag and drop these files back and forth several times, e.g.

1st dnd:  utils/m*.ts -> funcs/m*.ts
2st dnd:  funcs/m*.ts -> utils/m*.ts
3rd dnd:  utils/m*.ts -> funcs/m*.ts

You would see the states for these opened editors are in chaos. Some showed as "deleted", see the screenshot below. But actually these files exist. My initial finding was that vscode didn't clear the opened text models for the old locations after the move operation, so that the next time you move back, vscode will try to reuse the previously cached text models.

Does this issue occur when all extensions are disabled?: Yes

The text was updated successfully, but these errors were encountered:

isidorn · 2020-05-18T07:21:09Z

@bpasero sounds like this is on the file IO layer, and not in the UI representation. Can you investigate first? Let me know if you are busy and I can also investigate.

bpasero · 2020-05-18T09:14:50Z

@testforstephen I tried to reproduce but it seemed to work ok for me, maybe more settings are needed? For once, it is not clear to me if it breaks only in the one case where typescript.updateImportsOnFileMove.enabled is set to always or in both cases? It seems quite obvious to me that this issue only surfaces for you if the imports are updated, right?

Can you try the same thing in insiders, to rule out any particular extension or setting you might have that I don't. You can give our preview releases a try from: https://code.visualstudio.com/insiders/

bpasero · 2020-05-18T09:29:34Z

Hm, I still see a bit of weirdness: the title indicates the file is deleted, but the editor tab does not show that. I think there is no breakage of functionality or data loss (@testforstephen correct me if that is different for you) but there seems to be a bug in showing the decoration in that case.

testforstephen · 2020-05-18T10:19:37Z

At insider, i can reproduce it in both updateImports enabled and disabled.

I didn't observe data loss yet, but not sure whether some complex case will be.

At least, i observed there are some extra didChange events happening for those problematic editors. And if there is a job on applying the edit in the meantime, it probably throws errors saying there are files changed in the meantime. Especially if you choose to use Refactor Preview panel to preview the updates introduced by dnd, then apply the changes will most likely run into the error saying the file has been modified.

bpasero · 2020-05-18T11:40:41Z

So the fact that imports are still being rewritten when this setting is disabled smells like a bug that I filed as #98081

Checking for the state of file models when doing this, I see some stale text file models lurking around, easy to identify:

move those files back and forth as instructed
run the command "Developer: Log Working Copies"
notice how eventually there are working copies for the moved files both in the one folder as well as the other

Checking for callers of ITextModelResolverService#createReferencedObject() it looks like MainThreadDocuments#$tryCreateDocument() is responsible for creating a reference that is not being disposed when the file is moved.

@jrieken is that intentional? I see BoundModelReferenceCollection having some time associated with it, but this means in theory that invalid models are still hanging around when files get deleted or moved? I think that should be revisited and possibly the IWorkingCopyFileService#onDidRunWorkingCopyFileOperation be used to dispose these references.

jrieken · 2020-05-18T11:58:43Z

@jrieken is that intentional? I see BoundModelReferenceCollection having some time associated with it, but this means in theory that invalid models are still hanging around when files get deleted or moved?

Yes - we allow extensions to open text documents and there should be a little respecting of extensions when it comes to dispose. E.g. not dispose immediately but eventually. Also, dispose them when they never appear in an editor or as edited. So, today they remain for 3 mins or when a certain size constraint is reached.

This code is very ancient and maybe renaming/deleting wasn't an issue back then - if I remember correctly then the editor disposed models outside the ref-counted lifecycle for the longest time, right?

bpasero · 2020-05-18T12:19:07Z

if I remember correctly then the editor disposed models outside the ref-counted lifecycle for the longest time, right?

Maybe in the past, not sure. But for a while file editors use the same mechanisms and dispose the reference when the editor closes:

vscode/src/vs/workbench/contrib/files/common/editors/fileEditorInput.ts

Line 322 in 1fc148b

dispose(this.cachedTextFileModelReference);

I think just dipsosing the model under the hood without everyone that created a reference knowing about this is very dangerous because you may keep a reference to a model that is disposed without knowing.

I can make a pass over clients of the text model resolver that currently do not react on the move/delete and we can go from there. I will also check if my suggestion about participating in the working copy file event is possible, but I think it should be fine. This would then just be another condition under which we free up a reference, besides the timeout.

jrieken · 2020-05-19T06:53:48Z

Maybe in the past, not sure. But for a while file editors use the same mechanisms and dispose the reference when the editor closes:

vscode/src/vs/workbench/common/editor/textEditorModel.ts

Lines 96 to 98 in e5dadf5

    
           let model = resource && this.modelService.getModel(resource); 
        
           if (!model) { 
        
           	model = this.modelService.createModel(value, languageSelection, resource);

☝️ this is what I have meant. The text editor will re-use a model that it doesn't own so I wonder what happens in case the actual owner(s) let it go. E.g this

extension acquires model reference via API
editor opens and reuses that model (line 96)
extension reference is released (e.g after timeout) and model is disposed
editor uses a disposed model?

bpasero · 2020-05-19T07:34:25Z

That cannot happen, text editors go through the reference way as well, otherwise we would have seen a ton of issues by now. Text file editors always create a reference here:

vscode/src/vs/workbench/contrib/files/common/editors/fileEditorInput.ts

Line 253 in 6865124

    
           this.cachedTextFileModelReference = await this.textModelResolverService.createModelReference(this.resource) as IReference<ITextFileEditorModel>;

Still, the fact that we have code like this is dangerous, and possibly not used anymore or at least needs some better understanding. I can look into this as part of looking into the lifecycle of references.

#98057)

bpasero · 2020-05-19T09:58:36Z

I checked back on the fact that file models and untitled models have their own way of creating text models and it seems fine because ITextModelResolverService is actually going through that code:

untitled:

vscode/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts

Line 160 in 2aa9f3f

const model = await this.untitledTextEditorService.resolve({ untitledResource: resource });
file:

vscode/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts

Line 42 in 2aa9f3f

return this.textFileService.files.resolve(resource, { reason: TextFileLoadReason.REFERENCE });

There should be no issue from that usage that I can think of as long as everyone is going through these methods which they should.

…98154) * Dangling text file models of deleted files hanging around in memory (fix #98057) * address feedback

testforstephen · 2020-05-28T07:36:48Z

When i try the latest insider (2020-05-27), i still can reproduce the issue with the sample project above.

Version: 1.46.0-insider (user setup)
Commit: 876f2e70f9a2e1988887f8ca82294418afac15a2
Date: 2020-05-27T05:43:41.115Z
Electron: 7.2.4
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18363

Is the fix on the latest insider?

bpasero · 2020-05-28T08:28:10Z

Should be fixed, reopening then.

jrieken · 2020-06-04T08:51:09Z

Using the sample and steps below, this does seems to work for me. Tho, I wonder if we fixed the right issue because the code that @bpasero added isn't actually hit. Well the event fires but there is removal happening because the documents do exist on the extension host but they haven't been requested by an extension and are therefore not kept in the BoundReferenceCollection. This was still a good change but I wonder if we have missed something...

For more insights, I have an extension that logs all onDidOpen and onDidClose events for text documents and this is what it printed. At one point you see a series of CLOSE messages for the files that have been moved, that's followed by OPEN messages for the new locations, and finally come OPEN messages for the files that get updated

OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/tsconfig.json
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
OPEN: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
CLOSE: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
OPEN: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
CLOSE: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
OPEN: output:tasks
CLOSE: output:tasks
OPEN: output:extension-output-%234
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/tsconfig.json
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}

@testforstephen Did you try Insiders with this test case or is there a more complicated one in the Java world?

testforstephen · 2020-06-04T09:52:27Z

Open the sample nodejs project below in VS Code.
dnd.zip

Set the setting "typescript.updateImportsOnFileMove.enabled": "always".

Click to open multiple files (e.g. src/utils/ma.ts, src/utils/mb.ts, src/utils/mc.ts, src/utils/md.ts) in VS Code.

In File Explorer, drag and drop these files back and forth several times, e.g.

1st dnd:  utils/m*.ts -> funcs/m*.ts
2st dnd:  funcs/m*.ts -> utils/m*.ts
3rd dnd:  utils/m*.ts -> funcs/m*.ts

I tried the steps above in the latest insider 2020-06-03 again, the issue still exists. In fact, i also tried in a new windows machine, it has same issue.

@jrieken Did you check the window title? After DnD back and froth several times, you would see a (deleted) text in the window title.

In Java extension, what i want to do is same as "Update Imports on Move" feature in typescript. A little difference is that we provide the option for users to preview the update. The current problem is that after dragging files back and forth a few times, then applying preview will fail and it says "files were modified in the meantime". At the same time, i noticed there are additional title bar text "(deleted)" appended to these affected editors. Given it can also be reproduced in typescript, that's why i confirm it's a vscode issue.

jrieken · 2020-06-04T09:56:10Z

Oh, my bad. Didn't move back and forth, moved only once

jrieken · 2020-06-04T10:11:44Z

I can now reproduce but from the logs and debugging it seems that the extension host side is looking correct - I see meaningful CLOSE/OPEN messages and I do see how the remove function is executing.

Moving this to June for further investigation but @bpasero I believe there is more to this than just extension referenced documents

jrieken · 2020-06-04T10:13:08Z

A little difference is that we provide the option for users to preview the update.

FYI - our current plan is to add API, e.g a workspace edit can say that it can be previewed and based on user-preference the preview pane will show. Similar to how rename (symbol) preview works

bpasero · 2020-06-04T10:59:12Z

This seems to be the same underlying reason as #98653. During the steps we close and open editors, but some of them are inactive. When these get closed (e.g. by doing another DND operation), we do not dispose the model because an editor only disposes a model if it was opened actively, not inactive. Since the models are dirty doing the operation, we refuse to dispose it when the bulk edits are done.

I think there is no issue with disposal of references, but the sequence is rather this:

bulk edit creates model ref and performs edits
bulk edits is done and disposes the ref
we do not dispose the model because it is dirty at this point
at this time we have a model without associated reference that can potentially leak
we open dirty models by default in the background
usually editors participate in the lifecycle of models by creating a reference, but not when opened in background (we do not call resolve in that case)
operation is done again, since files are moved, we close editors and reopen them
closing an inactive editor does not dispose any reference because none was created

I am not sure yet about the proper fix but tested locally that a fix for #98653 should also fix this issue. Closing as duplicate.

github-actions bot added the new release label May 18, 2020

alexdima assigned bpasero and isidorn May 18, 2020

bpasero added the info-needed Issue requires more information from poster label May 18, 2020

bpasero unassigned isidorn May 18, 2020

bpasero added this to the May 2020 milestone May 18, 2020

bpasero added the workbench-editors Managing of editor widgets in workbench window label May 18, 2020

bpasero mentioned this issue May 18, 2020

TS: imports are being rewritten even if disabled in settings #98081

Closed

bpasero removed the info-needed Issue requires more information from poster label May 18, 2020

bpasero assigned jrieken May 18, 2020

bpasero changed the title ~~The opened editors are in chaos after several drag&drop operations~~ Dangling text file models of deleted files hanging around in memory May 18, 2020

bpasero added the debt Code quality issues label May 18, 2020

testforstephen mentioned this issue May 18, 2020

Enable refactor when moving files redhat-developer/vscode-java#1454

Closed

github-actions bot removed the new release label May 19, 2020

bpasero added a commit that referenced this issue May 19, 2020

Dangling text file models of deleted files hanging around in memory (fix

37e16aa

#98057)

bpasero mentioned this issue May 19, 2020

Dangling text file models of deleted files hanging around in memory #98154

Merged

bpasero closed this as completed in #98154 May 26, 2020

bpasero added a commit that referenced this issue May 26, 2020

Dangling text file models of deleted files hanging around in memory (#…

a23d4da

…98154) * Dangling text file models of deleted files hanging around in memory (fix #98057) * address feedback

bpasero reopened this May 28, 2020

jrieken modified the milestones: May 2020, June 2020 Jun 4, 2020

bpasero closed this as completed Jun 4, 2020

bpasero added the *duplicate Issue identified as a duplicate of another issue(s) label Jun 4, 2020

github-actions bot locked and limited conversation to collaborators Jul 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dangling text file models of deleted files hanging around in memory #98057

Dangling text file models of deleted files hanging around in memory #98057

testforstephen commented May 18, 2020

isidorn commented May 18, 2020 •

edited

Loading

bpasero commented May 18, 2020

bpasero commented May 18, 2020

testforstephen commented May 18, 2020

bpasero commented May 18, 2020

jrieken commented May 18, 2020

bpasero commented May 18, 2020

jrieken commented May 19, 2020 •

edited

Loading

bpasero commented May 19, 2020

bpasero commented May 19, 2020

testforstephen commented May 28, 2020

bpasero commented May 28, 2020

jrieken commented Jun 4, 2020

testforstephen commented Jun 4, 2020

jrieken commented Jun 4, 2020

jrieken commented Jun 4, 2020

jrieken commented Jun 4, 2020

bpasero commented Jun 4, 2020

Dangling text file models of deleted files hanging around in memory #98057

Dangling text file models of deleted files hanging around in memory #98057

Comments

testforstephen commented May 18, 2020

isidorn commented May 18, 2020 • edited Loading

bpasero commented May 18, 2020

bpasero commented May 18, 2020

testforstephen commented May 18, 2020

bpasero commented May 18, 2020

jrieken commented May 18, 2020

bpasero commented May 18, 2020

jrieken commented May 19, 2020 • edited Loading

bpasero commented May 19, 2020

bpasero commented May 19, 2020

testforstephen commented May 28, 2020

bpasero commented May 28, 2020

jrieken commented Jun 4, 2020

testforstephen commented Jun 4, 2020

jrieken commented Jun 4, 2020

jrieken commented Jun 4, 2020

jrieken commented Jun 4, 2020

bpasero commented Jun 4, 2020

isidorn commented May 18, 2020 •

edited

Loading

jrieken commented May 19, 2020 •

edited

Loading