Skip to content

Add cleanup code for projects and script infos #1007

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented May 31, 2025

Changes include:

  • Release unused scriptinfos during updateGraph
  • Release unused configured projects and script infos after open - this could be a go routine instead that does work after certain time and checking if no new projects are created or something - but for now following same thing as strada - after opening the file - which allows close -open which is a pattern in the
  • Made inferred projects as map for projectRoot to make it easier and use sync maps for configured and inferred projects to ensure concurrency safety
  • Added mutex for containing projects of scriptInfo
  • Configured project creation doesnt need rootFiles to have script info (Inferred does because it could be dynamic script info - but may be it doesnt given we dont have wierd project root and current directories to handle - TODO for later investigation)

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)

@sheetalkamat sheetalkamat marked this pull request as ready for review June 4, 2025 06:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

@@ -220,6 +222,10 @@ func newProjectServiceHost(files map[string]any) *ProjectServiceHost {
defaultLibraryPath: bundled.LibPath(),
ClientMock: &ClientMock{},
}
var watchCount atomic.Int32
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] If negative values are not expected for the counter, consider using atomic.Uint32 to semantically match a non-negative counter.

Suggested change
var watchCount atomic.Int32
var watchCount atomic.Uint32

Copilot uses AI. Check for mistakes.

Comment on lines +853 to +857
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")
Copy link
Preview

Copilot AI Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant