Add cleanup code for projects and script infos #1007
Conversation
sheetalkamat
force-pushed
the
cleanup
branch
9 times, most recently
from
fe72bc2 to
fdbb8fe
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces cleanup logic improvements for projects and script infos along with enhanced file watcher handling. Key changes include:
- Adding an atomic counter and watcher function in the test utility.
- Converting raw maps to collections.SyncMap and refactoring project/service methods.
- Improving cleanup, detachment, and configuration reload logic in project and script info management.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/testutil/projecttestutil/projecttestutil.go | Adds an atomic counter for file watchers and a customized WatchFilesFunc. |
| internal/project/watch.go | Refactors updateWorker error handling and logging for file watcher updates. |
| internal/project/service.go | Converts maps to SyncMap with Range iterations and refactors project assignment and cleanup logic. |
| internal/project/scriptinfo.go | Introduces mutex locking around containingProjects and adds accessor functions. |
| internal/project/project.go | Adjusts project lifecycle behavior including pendingReload, config loading, file removal, and detachment. |
| internal/project/ata.go | Minor code cleanup in initializer call. |
| internal/api/api.go | Updates LoadConfig call to match the new return signature. |
| internal/project/projectlifetime_test.go | Updates and adds tests for configured, inferred, and unrooted inferred projects. |
Comments suppressed due to low confidence (4)
internal/project/watch.go:59
- [nitpick] Consider simplifying or clarifying the log message to better indicate the state when an unwatch operation fails.
w.p.Log(fmt.Sprintf("%s:: Failed to unwatch %s watch: %s, err: %v newGlobs that are not updated: \n%s", w.p.name, w.watchType, w.watcherID, err, formatFileList(w.globs, "\t", hr)))
internal/project/service.go:628
- Changing the return type of assignProjectToOpenedScriptInfo from a struct to *Project is a significant API update; ensure that all callsites are updated accordingly.
func (s *Service) assignProjectToOpenedScriptInfo(info *ScriptInfo) *Project {
internal/project/project.go:871
- Since LoadConfig now returns a (bool, error) tuple, verify that all callsites properly handle both values and that pendingReload is reset appropriately after a successful reload.
func (p *Project) LoadConfig() (bool, error) {
internal/project/scriptinfo.go:136
- With the introduction of containingProjectsMu for guarding accesses, double-check that every access to containingProjects is correctly protected to avoid race conditions.
if s.isAttached(project) {
| func (p *Project) AddInferredProjectRoot(info *ScriptInfo) { | ||
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
| p.addRoot(info) | ||
| if p.isRoot(info) { | ||
| panic("script info is already a root") |
There was a problem hiding this comment.
[nitpick] The function panics if the script info is already a root; consider whether a graceful error handling strategy might be more appropriate for production usage.
Copilot uses AI. Check for mistakes.
|
How is it that the project service’s maps of projects can be mutated concurrently? I wonder if instead of using sync maps, we should be coordinating project synchronization to happen synchronously, like we do with document-editing requests. |
|
Yeah, I was a little confused by the introduction of syncmaps there. I mean it's great to do more stuff concurrently, but I wasn't sure if we needed to do that... |
|
I had that before but project creation of one isn’t dependent on another project .. so sync maps suffice in my opinion |
|
Are you saying project create , updates , deletes should’ve on same thread ? That seems unnecessary |
|
Presumably a project update would not change the map. I think we’d want different projects to be able to handle updates concurrently. But create/delete seems reasonable to synchronize and hold other requests back for. To me, the introduction of sync maps is not itself a problem, but I definitely want to understand why and how it’s needed, both to evaluate whether it’s worth the complexity and to understand what else needs to be guarded when making future changes. |
There was a problem hiding this comment.
Tested it out locally, and things seem to work as expected; I can see it opening new projects, then they disappear after I close files and start typing.
It'd be cool in the future to not gate things to updateGraph and so on, instead have some sort of delay for unused Projects/ScriptInfos to clear them out asynchonrously (since such a thing is now possible).
|
Yeah that’s a todo for later as I mentioned in the description |
Changes include:
Verified that memory usage goes down after opening 2-3 projects and then closing all of them and opening new one or file from one of them. (Currently memory usage is printed on open file in log)