x/tools/gopls: first Configuration does not happen as part of handling Initialized #35817
Milestone
Comments
myitcv
changed the title
|
Another reason this needs to happen as part of the handling of |
myitcv
added a commit
to govim/govim
that referenced
this issue
Nov 25, 2019
As a temporary, fragile workaround for golang/go#35817, we block the initialisation of govim until we have received the Configuration call from gopls. Per the TODO message in the code, this is fragile and best and built on broken assumptions at worst. But it should suffice for now.
myitcv
added a commit
to govim/govim
that referenced
this issue
Nov 25, 2019
As a temporary, fragile workaround for This is a rather fragile workaround for golang/go#35817, we block the initialisation of govim until we have received the Configuration call from gopls. It's fragile because we are relying on gopls not handling any requests until the response to Configuration is received and processed. In practice this appears to currently be the case but there is no guarantee of this going forward. Rather we hope that a fix for golang/go#35817 lands sooner rather than later at whic point this workaround can go. We also use a lock here because, despite it appearing that will only be a single Configuration call and that if there were more they would be serial, we can't rely on this. This is fragile and best and built on broken assumptions at worst. But it should suffice for now.
myitcv
added a commit
to govim/govim
that referenced
this issue
Nov 26, 2019
As a temporary, fragile workaround for This is a rather fragile workaround for golang/go#35817, we block the initialisation of govim until we have received the Configuration call from gopls. It's fragile because we are relying on gopls not handling any requests until the response to Configuration is received and processed. In practice this appears to currently be the case but there is no guarantee of this going forward. Rather we hope that a fix for golang/go#35817 lands sooner rather than later at whic point this workaround can go. We also use a lock here because, despite it appearing that will only be a single Configuration call and that if there were more they would be serial, we can't rely on this. This is fragile and best and built on broken assumptions at worst. But it should suffice for now.
myitcv
added a commit
to govim/govim
that referenced
this issue
Nov 27, 2019
As a temporary, fragile workaround for This is a rather fragile workaround for golang/go#35817, we block the initialisation of govim until we have received the Configuration call from gopls. It's fragile because we are relying on gopls not handling any requests until the response to Configuration is received and processed. In practice this appears to currently be the case but there is no guarantee of this going forward. Rather we hope that a fix for golang/go#35817 lands sooner rather than later at whic point this workaround can go. We also use a lock here because, despite it appearing that will only be a single Configuration call and that if there were more they would be serial, we can't rely on this. This is fragile and best and built on broken assumptions at worst. But it should suffice for now.
myitcv
added a commit
to govim/govim
that referenced
this issue
Nov 27, 2019
…593) As a temporary, fragile workaround for This is a rather fragile workaround for golang/go#35817, we block the initialisation of govim until we have received the Configuration call from gopls. It's fragile because we are relying on gopls not handling any requests until the response to Configuration is received and processed. In practice this appears to currently be the case but there is no guarantee of this going forward. Rather we hope that a fix for golang/go#35817 lands sooner rather than later at whic point this workaround can go. We also use a lock here because, despite it appearing that will only be a single Configuration call and that if there were more they would be serial, we can't rely on this. This is fragile and best and built on broken assumptions at worst. But it should suffice for now.
|
I expect that this is more likely an issue in the way that we are writing out the logs, because the initialized request definitely asks for configurations before proceeding. The |
I'm not entirely sure on that, because this was also confirmed by the Here's a similar snippet from our
|
|
Can you try reproducing this on a file with build tags or some other configuration you would expect |
|
@stamblerre kindly explained offline that this ordering of these notifications/calls is actually not significant, because all aspects in I therefore incorrectly attributed these seemingly "out of order" logs to be the root cause of #35933. But adding a sleep in Hence closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What version of Go are you using (
go version)?Note that
github.com/myitcvforks/tools v0.0.0-20191119111301-0222b4b716c6andgithub.com/myitcvforks/tools/gopls v0.0.0-20191119111301-0222b4b716c6correspond to thex/tools95cb2a1 with 80313e1 cherry picked on top. Reason being, we can't move beyond 95cb2a1 because otherwise we start tripping over the mistmatched versions problem described in #35114Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
We have a
govimtest that is looking to verify the behaviour of setting the"env"config setting withGOFLAGS=-mod=reaonly.However the issue we are seeing is that the initial
Configurationcall fromgoplstogovimhappens as follows:govim -> gopls: Initialize()govim -> gopls: Initialized()(notification)govim -> gopls: didOpen("main.go")govim -> gopls: didChange("main.go")gopls -> govim: Configuration()i.e. it's not happening as part of handling the
Initializednotification fromgovim.What did you expect to see?
This doesn't appear to be specified in the LSP spec https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#initialized but not calling
Configurationas part of handling theInitializednotification means that the client has no way of registering configuration that should be applied "from the start".e.g. in this case, setting
"env"to includeGOFLAGS=-mod=reaonly. If this does not happen during the handling ofInitializedthen thego.modfile will likely already have been modified by the time the results ofConfigurationcome to be processed.One could argue that the client could instead send the
DidChangeConfigurationnotification immediately afterInitializedbut this is not specified either, so we can't be guaranteed that all clients will do this. Whereas callingConfigurationas part of the handling ofInitialized(not afterwards because otherwise this creates a race) seems safe.What did you see instead?
As above.
goplslog as follows:fail.log
cc @stamblerre
The text was updated successfully, but these errors were encountered: