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.

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

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

Open
findleyr opened this issue Apr 16, 2020 · 5 comments
Open

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

findleyr opened this issue Apr 16, 2020 · 5 comments
Assignees

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 16, 2020

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 label Apr 16, 2020
@findleyr findleyr self-assigned this Apr 16, 2020
@gopherbot gopherbot added the gopls label Apr 16, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 16, 2020

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
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 label Jun 24, 2020
@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jun 24, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 25, 2020

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
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
Contributor Author

@findleyr 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

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 30, 2020

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.