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.

Sign up for GitHub

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

Jump to bottom

potential listener LEAK detected: at FileMatch.registerListeners #64120

Closed
bpasero opened this issue Dec 1, 2018 · 11 comments
Closed

potential listener LEAK detected: at FileMatch.registerListeners #64120

bpasero opened this issue Dec 1, 2018 · 11 comments
Assignees
@roblourens
Labels
bug Issue identified by VS Code Team member as probable bug search Search widget and operation issues verified Verification succeeded
Milestone
November 2018

Comments

@bpasero
Copy link
Member

bpasero commented Dec 1, 2018 •
edited

Steps to Reproduce:

  1. open vscode workspace
  2. search for "storageService"
at FileMatch.registerListeners (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:157:46)
    at FileMatch [as constructor] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:133:19)
    at create (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/types.js:164:14)
    at InstantiationService._createInstance (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/platform/instantiation/common/instantiationService.js:107:35)
    at InstantiationService.createInstance (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/platform/instantiation/common/instantiationService.js:76:31)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:407:66
    at Array.forEach (<anonymous>)
    at FolderMatch.add (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:394:17)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:596:33
    at Map.forEach (<anonymous>)
    at ResourceMap.forEach (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/map.js:428:22)
    at SearchResult.add (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:590:26)
    at SearchModel.onSearchProgress (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:857:36)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:792:23
    at onProviderProgress (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/search/node/searchService.js:102:25)
    at progressCallback (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/search/node/searchService.js:136:25)
    at Array.forEach (<anonymous>)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/search/node/searchService.js:517:45
    at Emitter.fire (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:223:38)
    at Emitter.fire (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:226:41)
    at handler (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/parts/ipc/node/ipc.js:380:59)
    at ChannelClient.onResponse (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/parts/ipc/node/ipc.js:432:17)
    at ChannelClient.onBuffer (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/parts/ipc/node/ipc.js:421:33)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/parts/ipc/node/ipc.js:285:91
    at Emitter.fire (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:223:38)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/parts/ipc/node/ipc.cp.js:146:44
    at Emitter.fire (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:223:38)
    at ChildProcess.fn (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/event.js:701:27)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at emit (internal/child_process.js:772:12)
    at _combinedTickCallback (internal/process/next_tick.js:141:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

/cc @jrieken
@bpasero
Copy link
Member Author

bpasero commented Dec 1, 2018 •
edited

Also when replacing I see this one:

at TextFileEditorModel.registerListeners (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textfile/common/textFileEditorModel.js:76:45)
    at TextFileEditorModel [as constructor] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textfile/common/textFileEditorModel.js:59:19)
    at create (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/types.js:164:14)
    at InstantiationService._createInstance (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/platform/instantiation/common/instantiationService.js:107:35)
    at InstantiationService.createInstance (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/platform/instantiation/common/instantiationService.js:76:31)
    at TextFileEditorModelManager.loadOrCreate (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textfile/common/textFileEditorModelManager.js:181:51)
    at ResourceModelCollection.createReferencedObject (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textmodelResolver/common/textModelResolverService.js:47:52)
    at ResourceModelCollection.ReferenceCollection.acquire (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/lifecycle.js:72:79)
    at TextModelResolverService._createModelReference (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textmodelResolver/common/textModelResolverService.js:135:52)
    at TextModelResolverService.createModelReference (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/textmodelResolver/common/textModelResolverService.js:120:25)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:185:79
    at Map.forEach (<anonymous>)
    at BulkEditModel.<anonymous> (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:184:41)
    at step (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:57:23)
    at Object.next (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:38:53)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:32:71
    at new Promise (<anonymous>)
    at __awaiter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:28:12)
    at BulkEditModel.prepare (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:173:20)
    at BulkEdit.<anonymous> (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:397:56)
    at step (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:57:23)
    at Object.next (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:38:53)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:32:71
    at new Promise (<anonymous>)
    at __awaiter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:28:12)
    at BulkEdit._performTextEdits (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:388:20)
    at BulkEdit.<anonymous> (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:305:59)
    at step (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:57:23)
    at Object.next (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:38:53)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:32:71
    at new Promise (<anonymous>)
    at __awaiter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:28:12)
    at BulkEdit.perform (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:265:20)
    at BulkEditService.apply (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js:469:29)
    at ReplaceService.replace (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/browser/replaceService.js:111:43)
    at SearchResult.replaceAll (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/common/searchModel.js:619:47)
    at file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/parts/search/browser/searchView.js:332:50
    at Object.notifySuccess [as _notify] (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1191:59)
    at Object.enter (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:867:30)
    at _Base.Class.derive._creator._run (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1089:29)
    at _Base.Class.derive._creator._completed (file:///Users/bpasero/Development/Microsoft/monaco/out/vs/base/common/winjs.base.js:1057:18)
    at <anonymous>

@bpasero bpasero assigned roblourens and jrieken and unassigned roblourens Dec 1, 2018
@bpasero
Copy link
Member Author

bpasero commented Dec 1, 2018

Not sure if the leak warning needs to be adjusted for this case or search should somehow install global listener for all results instead of one per result.

@roblourens
Copy link
Member

The first one, if it's an issue we can register the listener globally or once per folder instead of once per file.

The second I think is expected because we have to open all the files to perform the replace. cc @sandy081

Maybe we need a way to disable the warning for certain calls.

@roblourens
Copy link
Member

Also the leak check is not free, maybe that's a +1 for reducing the number of calls

image

@jrieken jrieken removed their assignment Dec 3, 2018
@jrieken
Copy link
Member

jrieken commented Dec 4, 2018

Also the leak check is not free, maybe that's a +1 for reducing the number of calls

That's because it captures a stack trace. Not sure how often events have been registered to get to 33ms. Please file a new issue when you see this often, we can then disable the warning for stable builds

@roblourens
Copy link
Member

This was for a typical text search. But what do you think I should be doing here, ignore the warning for now, move the listener to be registered less often, or come up with a way to disable it for this particular instance?

@jrieken
Copy link
Member

jrieken commented Dec 5, 2018 •
edited

or come up with a way to disable it for this particular instance?

You can already do that - each emitter can be configured individually using the EmitterOptions. However, having the container listen instead of its children is often the better approach, esp when all listeners are the same.

@roblourens
Copy link
Member

I mean instance of the listener, but yeah moving the listener is better, I will do that in debt week.

@roblourens roblourens added debt Code quality issues search Search widget and operation issues labels Dec 5, 2018
@roblourens roblourens added bug Issue identified by VS Code Team member as probable bug and removed debt Code quality issues labels Dec 5, 2018
@roblourens
Copy link
Member

Actually the warning is not a good look, I'll do it in November...

@bpasero
Copy link
Member Author

bpasero commented Dec 6, 2018

@roblourens there are still many more warnings printed when doing a search+replace over > 100 files because we easily create more than 100 text models for the text edit and tons of listeners are being created temporarily. Honestly I do not even know how to fix that other than maybe disposing the models in chunks of a 100 while doing the replace operation (/cc @sandy081 @jrieken)

@sandy081
Copy link
Member

sandy081 commented Dec 6, 2018

Its also the same when we do rename that touches hundreds of files. No idea how to fix such.

@mjbvz mjbvz added the verified Verification succeeded label Dec 10, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@roblourens roblourens

Labels
bug Issue identified by VS Code Team member as probable bug search Search widget and operation issues verified Verification succeeded
Projects
None yet
Milestone
November 2018
Development

No branches or pull requests
5 participants
@roblourens @bpasero @jrieken @sandy081 @mjbvz