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

x/tools/go/packages: golist driver leaks a go command invocation #65180

Closed
findleyr opened this issue Jan 19, 2024 · 4 comments
Closed

x/tools/go/packages: golist driver leaks a go command invocation #65180

findleyr opened this issue Jan 19, 2024 · 4 comments
Assignees
@adonovan
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@findleyr
Copy link
Contributor

Looking at the following improper shutdown in gopls:

https://build.golang.org/log/bf996ae51d525c51272637629e8dfe6ed4cb79a6

Investigating the goroutine dump there, it looks like the golist driver doesn't handle cancellation properly, and as a result leaks a goroutine that is running the Go command (which, in turn, prevents clean up of the module cache).

Specifically, the driver is here: https://cs.opensource.google/go/x/tools/+/master:go/packages/golist.go;l=154;drc=f60f2e6aa42c945111053771ea53938694791d83

But note that there are early returns that don't await that WaitGroup. My guess is that there's a race when the context is cancelled.

CC @matloob @adonovan
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 19, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 19, 2024
@findleyr findleyr mentioned this issue Jan 19, 2024
Closed
@matloob
Copy link
Contributor

matloob commented Jan 22, 2024

Should we defer a function that waits on the sizeswg and sets the error?

@findleyr
Copy link
Contributor Author

Not sure how important this concurrency is, but can we move the sizes query below the extractQueries loop and do all out-of-process work in an errgroup? Can runContainsQueries execute concurrently to the createDriverResponse on line 197? I started looking into this and then realized I should tread carefully and file this issue.

@matloob
Copy link
Contributor

matloob commented Jan 22, 2024

Yeah I don't think it should make a big difference to move the sizes query below the extractQueries loop. They both manipulate the same fields on the response so we'd have to be careful with it but I suppose we can lock the response when we access it?

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 23, 2024
@findleyr findleyr mentioned this issue Feb 1, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/560478 mentions this issue: go/packages: wait for sizes goroutine on all control paths

@adonovan adonovan self-assigned this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@adonovan @gopherbot @cherrymui @matloob @findleyr