Skip to content

x/tools/internal/lsp/regtest: closing workspace sometimes fails on Windows #38490

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
Closed
findleyr opened this issue Apr 16, 2020 · 7 comments
Closed

x/tools/internal/lsp/regtest: closing workspace sometimes fails on Windows #38490

findleyr opened this issue Apr 16, 2020 · 7 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
gopls/v0.9.2

Comments

@findleyr
Copy link
Member

Due to Windows' default file locking behavior, and the fact that gopls often shells out to the go command, closing our regtest workspaces (./internal/lsp/fake.Workspace) sometimes fails on windows with an error message like this:

C:\Users\gopher\AppData\Local\Temp\1\goplstest-ws-regtest-857411125: The process cannot access the file because it is being used by another process

In https://golang.org/cl/228231 I try to avoid this by deferring workspace cleanup until the end of the regtest test suite, but this is just a workaround and may break in the future. Also, this doesn't fix the problem for other test suites, such as ./internal/lsp/lsprpc.

One possible solution is to simply retry this cleanup until it succeeds or times out, but this is bound to be problematic. In testing it can take anywhere from 10ms to 1s for such a retry to succeed, and the retry behavior introduces a bunch of complexity that should be unnecessary.

A better solution may be related to something else we've discussed: ensure that all work done on behalf of gopls is captured in a span, and expose an API that can signal when gopls work has completed (for a workspace? for a snapshot?). If that is not sufficient, we may want to extend this to a control plane that allows (among other things) manually terminating any processes associated with a workspace.

/cc @stamblerre @ianthehat
@gopherbot gopherbot added this to the Unreleased milestone Apr 16, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Apr 16, 2020
@findleyr findleyr self-assigned this Apr 16, 2020
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Apr 16, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/228231 mentions this issue: internal/lsp/fake: be more careful when closing the workspace

@golang golang deleted a comment from gopherbot Apr 16, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Apr 16, 2020
@findleyr
internal/lsp/fake: be more careful when closing the workspace
405595e 
Closing the workspace has frequently been failing on Windows, due to
file locks held by the go command.

This change makes several tests more careful to check errors when
closing resources, and defers closing the regtest workspaces until the
entire test suite completes, at which point it is much more likely that
closing the workspace will succeed.

If this change results in test flakes on Windows, we should temporarily
demote errors in regtest.Runner.Close to a t.Log.

Updates golang/go#38490

Change-Id: Ibd2f7dd0e0e2faecfa0ca8c60237fc72e64f6719
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228231
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 Apr 21, 2020
@stamblerre stamblerre added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jun 24, 2020
@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jun 24, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/240059 mentions this issue: internal/lsp/regtest: use a common directory for regtest sandboxes

gopherbot pushed a commit to golang/tools that referenced this issue Jul 9, 2020
@findleyr
internal/lsp/regtest: use a common directory for regtest sandboxes
e404ca2 
For easier debugging (and less cruft if regtests are ctrl-c'ed), root
all regtest sandboxes in a common directory.

This also tries one last time to clean up the directory, and fails on an
error. This might be flaky on windows, but hasn't been so far...

Also give regtest sandboxes names derived from their test name.

Updates golang/go#39384
Updates golang/go#38490

Change-Id: Iae53c29e75f5eb2b8d938d205fbeb463ffc82eb2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/240059
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@findleyr
Copy link
Member Author

findleyr commented Aug 3, 2020

This is in fact still a problem after the above change: https://storage.googleapis.com/go-build-log/85afa2eb/windows-amd64-2016_3d74be50.log

Need to make this fail silently again. CC @heschik

@stamblerre
Copy link
Contributor

Saw this recently: https://storage.googleapis.com/go-build-log/2333c629/windows-386-2008_d05c3521.log.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/258315 mentions this issue: gopls/internal/regtest: allow cleanup to fail on windows

gopherbot pushed a commit to golang/tools that referenced this issue Sep 30, 2020
@findleyr
gopls/internal/regtest: allow cleanup to fail on windows
a6f32d1 
Due to Windows' default file locking and the fact that regtests shell
out to the go command, cleanup sometimes fails.

This is causing trybot flakes, increasingly as of late. Since the
tempdir will eventually be cleaned up on the trybots anyway, don't fail
on windows.

For golang/go#38490

Change-Id: I136d97143baba1d98777db51daa062cf0e42e33e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/258315
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
@ainar-g ainar-g mentioned this issue Mar 11, 2021
Closed
@bcmills
Copy link
Contributor

bcmills commented Jul 18, 2022

@findleyr, was this fixed by CL 417117?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 18, 2022
@findleyr
Copy link
Member Author

Just saw this. Yes, this was fixed!

@golang golang locked and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
gopls/v0.9.2
Development

No branches or pull requests
4 participants
@bcmills @stamblerre @gopherbot @findleyr