fix: auto patch for file-watcher_93 #4231
Conversation
dosubot
bot
added
size:S
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
Hey @kiwina, thank you again for your contribution.
I just have a couple of questions/suggestions for this one, let me know what you think about them!
| this.stopWatcher() // This calls _orchestrator.stopWatcher() | ||
| // Ensure orchestrator is fully cleaned up if it has a dispose method | ||
| if (typeof (this._orchestrator as any).dispose === "function") { | ||
| ;(this._orchestrator as any).dispose() |
There was a problem hiding this comment.
It looks like _orchestrator.stopWatcher() might be called twice here: once via this.stopWatcher() on line 219, and then again when this._orchestrator.dispose() is called on line 222 (since CodeIndexOrchestrator.dispose() also calls stopWatcher()).
Is this intentional?
I think to make this simpler we could have CodeIndexManager.dispose() rely primarily on this._orchestrator.dispose() to handle its own cleanup:
public dispose(): void {
if (this._orchestrator) {
// The orchestrator's dispose should handle its own stopWatcher call
if (typeof this._orchestrator.dispose === 'function') {
this._orchestrator.dispose();
} else {
// Fallback if for some reason dispose isn't on the orchestrator
this.stopWatcher();
}
}
this._stateManager.dispose();
}What are your thoughts on this approach?
There was a problem hiding this comment.
I belive everything should be unified with dispose method, just like some of the other PR.
If that is the case than stopWatcher is actually not even necessay. In the long run that would make contribution/testing across the board more unified
There was a problem hiding this comment.
Also
public stopWatcher(): void { if (!this.isFeatureEnabled) { return } if (this._orchestrator) { this._orchestrator.stopWatcher() } }
this._orchestrator needs to be disposed 100% stopWatcher() does not guarantee that
| if (typeof (this._orchestrator as any).dispose === "function") { | ||
| ;(this._orchestrator as any).dispose() |
There was a problem hiding this comment.
I see this._orchestrator is typed as CodeIndexOrchestrator | undefined (on line 24), and CodeIndexOrchestrator now has a dispose() method.
Given this, the as any cast on line 221 might not be strictly necessary. Was it included for a specific reason?
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Changes Requested]
in Roo Code Roadmap
|
Hi @kiwina, thanks for your contribution and for taking the time to address the FileWatcher leak concern! After reviewing the changes and the existing code, I believe the FileWatcher is already being properly disposed through the current implementation:
The changes in this PR add a If there's a specific scenario where you've observed the FileWatcher not being disposed, could you share more details? That would help us identify if there's a different root cause we need to address. For now, I'll temporarily close this PR as the disposal mechanism is already functioning properly. However, if you'd like to improve the disposal pattern for better consistency with VSCode conventions (using What do you think? |
github-project-automation
bot
moved this from PR [Changes Requested]
to Done
in Roo Code Roadmap
2 participants
PR for FileWatcher Leak (file-watcher_93)
This PR addresses the potential
FileSystemWatcherleak identified inleak-report/guaranteed/file-watcher_93.md.Description
The
FileWatchercreates aFileSystemWatcherthat might not be disposed if theCodeIndexManager's disposal chain is interrupted. WhileCodeIndexManageris added tocontext.subscriptions(which should ensure itsdisposemethod is called on extension deactivation), this patch aims to add robustness to the disposal process within theCodeIndexOrchestratoras a defensive measure.The proposed fix involves ensuring
CodeIndexOrchestratorhas an explicitdisposemethod that reliably callsstopWatcher()(which in turn disposes theFileWatcher).Related Bug Report
Closes: #4230
Changes
dispose()method insrc/services/code-index/orchestrator.tsto ensurestopWatcher()is called.CodeIndexManager.dispose()calls the orchestrator's dispose method if available (this might be redundant but adds safety).How to Test
CodeIndexManager.dispose()is called upon extension deactivation.CodeIndexManager.dispose()->CodeIndexOrchestrator.stopWatcher()(ordispose()) ->FileWatcher.dispose().Important
Fixes potential
FileSystemWatcherleak by ensuring proper disposal inCodeIndexOrchestratorandCodeIndexManager.CodeIndexOrchestrator.dispose()callsstopWatcher()to disposeFileWatcher.CodeIndexManager.dispose()now checks and callsCodeIndexOrchestrator.dispose()if available.dispose()method inorchestrator.tsto callstopWatcher().dispose()method inmanager.tsto ensure orchestrator disposal.CodeIndexManager.dispose()->CodeIndexOrchestrator.stopWatcher()->FileWatcher.dispose().This description was created by
for 5014270. You can customize this summary. It will automatically update as commits are pushed.