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/gopls: scaling multi-module workspaces #41558

Open
myitcv opened this issue Sep 22, 2020 · 21 comments
Open

x/tools/gopls: scaling multi-module workspaces #41558

myitcv opened this issue Sep 22, 2020 · 21 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Sep 22, 2020

What version of Go are you using (go version)?

$ go version
go version devel +b4ea672009 Mon Sep 21 01:30:48 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200918232735-d647fc253266
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200918232735-d647fc253266

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/.vim/plugged/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build208776869=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Opened govim in my home directory.

What did you expect to see?

No error messages from gopls

What did you see instead?

Error loading packages: open /home/myitcv/.dbus: permission denied

Log file: bad.log


cc @stamblerre

FYI @leitzler

@gopherbot gopherbot added the Tools label Sep 22, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 22, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Sep 22, 2020

Nothing in the entire Go codebase uses dbus. You have some exotic setup involving a wrapper for the go command, an LD_PRELOAD, or a gopackagesdriver.

@stamblerre stamblerre removed this from the Unreleased milestone Sep 22, 2020
@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 22, 2020

You have some exotic setup involving a wrapper for the go command, an LD_PRELOAD, or a gopackagesdriver.

What makes you think this? I don't actually have any of the above. Per the log file, I've simply opened Vim in my home directory.

FYI I bisected this to 78fed78f7d8ce6b9d4b47fdf34e6735dfb070fcb

@heschik
Copy link
Contributor

@heschik heschik commented Sep 22, 2020

Sorry, I assumed that this was an error from the Go command. We probably need better error messages in this area. I assume you've enabled ExperimentalWorkspaceModule?

@stamblerre, I guess this is a question of whether we should be ignoring errors while walking directories looking for go.mod files.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 22, 2020

I assume you've enabled ExperimentalWorkspaceModule?

No I haven't

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2020

Change https://golang.org/cl/256578 mentions this issue: internal/lsp: ignore errors when finding workspace modules

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 22, 2020

Ah, I specifically changed it so that we would always look for workspace modules even without ExperimentalWorkspaceModule so that we could catch any bad behavior in that code :) It sounds like we should be ignoring errors--mailed a CL.

@heschik
Copy link
Contributor

@heschik heschik commented Sep 22, 2020

I don't think that's good practice -- this code is known to be at least somewhat unstable, and if it's not guarded behind a flag we run the risk of breaking users the next time we release. We got lucky that Paul noticed this first.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 23, 2020

I don't think that's good practice

I'd tend to agree.

Is the plan with this new workspace module, when enabled, to recursively scan the directory in which the editor/gopls is opened?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 23, 2020

Previous discussion of a related topic/issue: #35818

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 23, 2020

I don't think that's good practice

I'd tend to agree.

While it's true that users shouldn't see issues caused by experimental features while working the default mode, I'm not too concerned about this particular case. This seems like something we wouldn't have tested for, so it's really a matter of catching it now versus catching it before we enable multi-module workspaces by default. I do agree that we're lucky Paul noticed -- thank you for reporting!

Is the plan with this new workspace module, when enabled, to recursively scan the directory in which the editor/gopls is opened?

Yep--we are now walking the workspace root looking for go.mod files.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 23, 2020

Yep--we are now walking the workspace root looking for go.mod files.

Is this approach going to scale when, as I did, someone opens an editor in their home directory?

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 23, 2020

I guess I'm not totally clear on the benefits of such a scan, vs adding modules when files from a module are opened in gopls:

  • on my machine a find $HOME -name go.mod takes ~2 mins from cold
  • newly created modules will not be automatically discovered unless we rescan or manually add them
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 23, 2020

I'm sorry, the issue got closed when I merged the CL. I'll reopen it and link it to the multi-module workspace umbrella issue (#32394), since this sounds more like a fundamental discussion about the multi-module workspace design.

To answer your question--the idea behind the scan is to support features like diagnostics and find-references, etc. across multiple or nested modules automatically. It may be a good idea to consider factoring in "depth" however--you're right that someone who opens their home directory would probably not want an enormous multi-module workspace. We could potentially incrementally add modules to the workspace as files are opened, but that does open up the possibility of non-deterministic results for features like find references. And monorepos might have many modules nested below a top-level directory, so distinguishing these cases would be difficult. As of right now, opening your $HOME directory in the multi-module workspace world is the equivalent of opening your entire $GOPATH--use at your own risk.

What do you think, @heschik?

@stamblerre stamblerre reopened this Sep 23, 2020
@stamblerre stamblerre changed the title x/tools/gopls: permission denied error message x/tools/gopls: scaling multi-module workspaces Sep 23, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Sep 23, 2020

I don't think we've spent enough time designing the UX for multi-module mode, so I don't think I have enough groundwork to give a good answer. I don't think a hardcoded limit either on depth or module count is the answer though. My first thought is that we should ask the user what they want at startup, but that could be annoying.

I believe that the previous behavior would have been for the folder to be detected as a misconfigured workspace, and then we'd do a Load on . rather than ./.... Why do we do that rather than failing to initialize? What use case does that serve, that this would presumably break?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 23, 2020

I believe that the previous behavior would have been for the folder to be detected as a misconfigured workspace, and then we'd do a Load on . rather than ./.... Why do we do that rather than failing to initialize? What use case does that serve, that this would presumably break?

I think the goal of that was to spare the user the cost of an initial workspace load on every module beneath their $HOME directory. Asking the user what they want would be reasonable if we could could cache their answer on disk, but even then, a user might change their mind later on, so we'd need to have the UX for them to switch. Maybe if we discover some very large number of modules (10+?), we could warn the user that we're about to load them before we do so?

@stamblerre stamblerre added this to the gopls/v1.0.0 milestone Sep 24, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 24, 2020

Change https://golang.org/cl/257137 mentions this issue: internal/lsp: don't search for workspace modules by default

@myitcv
Copy link
Member Author

@myitcv myitcv commented Sep 24, 2020

My first thought is that we should ask the user what they want at startup

This feels like a sensible starting point.

Maybe if we discover some very large number of modules (10+?), we could warn the user that we're about to load them before we do so?

Isn't the issue that with such an approach you will load a totally arbitrary selection of the modules beneath the working directory that may or may not (likely not) coincide with what the user wants?

Asking the user what they want would be reasonable if we could could cache their answer on disk, but even then, a user might change their mind later on, so we'd need to have the UX for them to switch

To my mind, this goes all the way back to the conversation we had a GopherCon last year, the UI/UX around workspace configuration in an editor. Because simply having a module open does not imply (to my understanding) that the user does want it to be part of the multi module magic, hence I guess you reference to "have the UX for them to switch"?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 24, 2020

One piece of configuration that we will offer is the user-defined gopls.mod file -- https://github.com/golang/proposal/blob/master/design/37720-gopls-workspaces.md#configuration. This will allow users to choose which modules they want gopls to load in a given directory. I think this would probably address a lot of the concerns here. We could show a warning that we're loading a lot of modules, and if you find it slow, you can change your gopls.mod file to only choose the ones you want.

gopherbot pushed a commit to golang/tools that referenced this issue Sep 24, 2020
This may be expensive, and we don't want to do it if the results will
be unused.

Updates golang/go#41558

Change-Id: I0c01a80f4fc459e1cc189934ad23ab443e5ea2a5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/257137
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@stamblerre stamblerre added this to Critical in gopls/v1.0.0 Sep 25, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Oct 7, 2020

As an update, in addition to recommending a gopls.mod in large workspaces, we will stop searching the workspace after some limit (10000 files searched, for example) and notify the user that their workspace is too large. We will fall back to loading the ./ query that we use for an invalid workspace.

The notification to the user will suggest that they either:

  • Do not open that directory again, or
  • Create a gopls.mod file that specifies the modules they would like to load

The notification will link to a document explaining how the gopls.mod configuration works.

@pierrre
Copy link

@pierrre pierrre commented Oct 7, 2020

Will it be possible to configure this limit ?
I guess this limit will help to keep a low memory usage or make the process faster.
However it could be helpful for some developers to increase this limit, event if it uses more memory or is slower.
Currently in my workspace I've got ~4000 .go files.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Oct 8, 2020

I don't think that the limit will be configurable. Instead, we will request that users that hit the limit create a gopls.mod file. This will cause gopls to bypass the module search, and multi-module workspace mode will continue working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

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