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/gopls: imports: nil panic in ClearModuleInfo (via telemetry) #66490

Closed
adonovan opened this issue Mar 22, 2024 · 4 comments
Closed

x/tools/gopls: imports: nil panic in ClearModuleInfo (via telemetry) #66490

adonovan opened this issue Mar 22, 2024 · 4 comments
Assignees
@adonovan
Labels
gopls/imports gopls/telemetry-wins gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.15.3

Comments

@adonovan
Copy link
Member

This stack pUwreg was reported by telemetry:

ClearModuleInfo:+4 is:

		if resolverErr == nil {
			<-r.scanSema // acquire (guards caches)                            HERE
			resolver.moduleCacheCache = r.moduleCacheCache
			resolver.otherCache = r.otherCache
			r.scanSema <- struct{}{} // release
		}

but I don't see at a glance how a ModuleResolver can be constructed without a valid scanSema.

Possibly related to https://go.dev/cl/561235?

crash/crash
runtime.gopanic:+69
runtime.panicmem:=261
runtime.sigpanic:+19
golang.org/x/tools/internal/imports.(*ProcessEnv).ClearModuleInfo:+4
golang.org/x/tools/gopls/internal/cache.(*importsState).runProcessEnvFunc:+25
golang.org/x/tools/gopls/internal/cache.(*Snapshot).RunProcessEnvFunc:=439
golang.org/x/tools/gopls/internal/golang.allImportsFixes:+4
golang.org/x/tools/gopls/internal/golang.CodeActions:+13
golang.org/x/tools/gopls/internal/server.(*server).CodeAction:+95
golang.org/x/tools/gopls/internal/protocol.serverDispatch:+160
golang.org/x/tools/gopls/internal/lsprpc.(*streamServer).ServeStream.ServerHandler.func3:+5
golang.org/x/tools/gopls/internal/lsprpc.(*streamServer).ServeStream.handshaker.func4:+52
golang.org/x/tools/gopls/internal/protocol.Handlers.MustReplyHandler.func1:+2
golang.org/x/tools/gopls/internal/protocol.Handlers.AsyncHandler.func2.2:+3
runtime.goexit:+0

golang.org/x/tools/gopls@v0.15.2 devel darwin/arm64 vscode (1)

Issue created by golang.org/x/tools/gopls/internal/telemetry/cmd/stacks.
@adonovan adonovan added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. gopls/telemetry-wins labels Mar 22, 2024
@gopherbot gopherbot added this to the Unreleased milestone Mar 22, 2024
@adonovan
Copy link
Member Author

This crash is impossible according to the Go spec. I suspect a bug in the Go toolchain that was used, which is not a released one.

@adonovan
Copy link
Member Author

I spoke too soon: it's a classic nil-pointer-makes-non-nil interface bug. Fix pending.

@adonovan adonovan reopened this Mar 25, 2024
@adonovan adonovan self-assigned this Mar 25, 2024
@adonovan adonovan removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 25, 2024
@gopherbot
Copy link

Change https://go.dev/cl/574136 mentions this issue: internal/imports: fix two "nil pointer in interface" bugs

@adonovan adonovan mentioned this issue Mar 25, 2024
Open
This was referenced Mar 25, 2024
Closed
Closed
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.15.3 Apr 2, 2024
@gopherbot
Copy link

Change https://go.dev/cl/577297 mentions this issue: [gopls-release-branch.0.15] internal/imports: fix two "nil pointer in interface" bugs

@findleyr findleyr mentioned this issue Apr 8, 2024
Closed
gopherbot pushed a commit to golang/tools that referenced this issue Apr 8, 2024
@adonovan @findleyr
[gopls-release-branch.0.15] internal/imports: fix two "nil pointer in…
5a4dc7e 
… interface" bugs

CL 559635 changed newModuleResolver so that it can return
(nil, err). That means it is no longer safe to unconditionally
convert the first result to a Resolver interface, but we
forgot to check in two places, causing a crash that was
reported by telemetry.

This change adds the two checks.

Updates golang/go#66490
Updates golang/go#66730

Change-Id: I3f2b84ed792b1eea179fc0d4d5ee9843281506fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/574136
Reviewed-by: Peter Weinberger <pjw@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 63b3b5a)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577297
Reviewed-by: Alan Donovan <adonovan@google.com>
This was referenced Apr 11, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
gopls/imports gopls/telemetry-wins gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.15.3
Development

No branches or pull requests
3 participants
@adonovan @gopherbot @findleyr