-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Provide JSON-RPC service for Language Service Communications #7840
Conversation
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature!
Can you take a look at my comments?
@@ -44,6 +55,12 @@ | |||
public class LanguageServerRegistryImpl implements LanguageServerRegistry { | |||
|
|||
private static final Logger LOG = LoggerFactory.getLogger(LanguageServerRegistryImpl.class); | |||
|
|||
private static final String WORKSPACE_ID = System.getenv("CHE_WORKSPACE_ID"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using Guice injection of this env var for consistency.
private Workspace getWorkspaceConfiguration() { | ||
UriBuilder builder = | ||
fromUri(apiEndpoint) | ||
.path(WorkspaceService.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we recently got weird behavior of such a code when built URL contained duplicated workspace
word and @skabashnyuk removed .path(WorkspaceService.class)
part.
@@ -345,4 +376,23 @@ public InitializedLanguageServer getServer(String id) { | |||
} | |||
return null; | |||
} | |||
|
|||
private Workspace getWorkspaceConfiguration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE has Workspace API client (WorkspaceServiceClient). Can you reuse it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to, but unfortunately no. It is interesting for me to know what, from your point of view, allows us to assume that this client code can be used on the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we have a client for WS API, but forgot that it is a client-side only thing
@@ -189,8 +212,16 @@ public ServerCapabilities initialize(String fileUri) throws LanguageServerExcept | |||
if (language == null) { | |||
return Collections.emptyList(); | |||
} | |||
List<LanguageServerLauncher> combinedLaunchers = new LinkedList<>(launchers); | |||
Workspace workspace = getWorkspaceConfiguration(); | |||
if (workspace != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on when this may happen and what the behavior of the wsagent would be in such a case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if you're asking. Please take a look at this code fragment:
try {
return httpJsonRequestFactory.fromUrl(href).useGetMethod().request().asDto(WorkspaceDto.class);
} catch (IOException | ApiException e) {
return null;
}
I must admit that at first glance it is not obvious where null
comes from, but if you could look a bit deeper you could see that this line
httpJsonRequestFactory.fromUrl(href).useGetMethod().request().asDto(WorkspaceDto.class);
could throw exceptions and that would result in null
.
Speaking about the consequences, I can assume that such situation is not something that we expect to happen, but in case it happens according to these condition
workspace != null
we will skip the next block of code
for (RemoteLsLauncherProvider launcherProvider : launcherProviders) {
combinedLaunchers.addAll(launcherProvider.getAll(workspace));
}
In case you are interested in those exceptions, I would recommend you to read javadoc of org.eclipse.che.api.core.rest.HttpJsonRequest#request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you for the explanation
@@ -189,8 +212,16 @@ public ServerCapabilities initialize(String fileUri) throws LanguageServerExcept | |||
if (language == null) { | |||
return Collections.emptyList(); | |||
} | |||
List<LanguageServerLauncher> combinedLaunchers = new LinkedList<>(launchers); | |||
Workspace workspace = getWorkspaceConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert in this area of Che, but by looking at a method argument fileUri
I suppose that this method can be called a lot of times because of different files. If this is true then looks like this code might call Workspace API a lot of times. Wouldn't it be better to cache GET WS API calls results?
} | ||
|
||
LanguageServerDescription extract(Map<String, String> attributes) { | ||
String config = attributes.get("config"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed before I would opt for a more FQN like name because the current one is not very descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we look at configuration fragment:
{
"environments":{
"default":{
"warnings":[
],
"machines":{
"typescript-ls-machine":{
"env":{
},
"installers":[
"org.eclipse.che.exec",
"org.eclipse.che.terminal"
],
"servers":{
"ls":{
"attributes":{
"internal":"true",
"type":"ls",
"config":"{\"id\":\"org.eclipse.che.plugin.web.typescript\", \"documentFilters\":[ { \"languageId\":\"typescript\", \"pathRegex\":\".*\\\\.(ts|tsx)\"} ]}"
},
"protocol":"tcp",
"port":"4417"
}
},
"volumes":{
"projects":{
"path":"/projects"
}
},
"attributes":{
"memoryLimitBytes":"1073741824"
}
}
}
}
}
}
we see that environments
have machines
which have servers
which have attributes
so from this perspective it looks descriptive enough and the logical FQN is something like default->typescript-ls-machine->ls->config
. On the other hand in the code in the decription it is mentioned: "class is responsible for language server description extraction out of server attributes map" so in underlined code line attributes.get("config")
it should be rather obvious that keyword cofig
is related to server's attributes while server is related to machin and so on. Again I'm wondering do you think that it is just config
not descriptive or internal
and type
are not descriptive as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned about attributes only since they do not have a fixed structure. And yes, I was thinking the same about internal
and type
.
* json corresponds to {@link LanguageServerDescription} class with all aggregated classes | ||
*/ | ||
@Singleton | ||
class LsConfigurationExtractor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much more complex than parsing to an object. Have you considered creating a model object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had some consideration for creating a model object.
|
||
/** Provides socket based language server launchers */ | ||
@Singleton | ||
class SocketLsLauncherProvider implements RemoteLsLauncherProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename it to TCP socket because there is a named socked concept that is also used by some LSs, even though it is rarely used?
@Override | ||
public Set<LanguageServerLauncher> getAll(Workspace workspace) { | ||
for (Map.Entry<String, ? extends Machine> machineEntry : | ||
workspace.getRuntime().getMachines().entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI runtime can be null in certain cases
@@ -107,7 +113,7 @@ protected String extractProjectPath(String filePath) throws LanguageServerExcept | |||
.thenAnswer(invocation -> completedFuture(Pair.of(languageServer, initializeResult))); | |||
} | |||
|
|||
@Test | |||
@Test(enabled = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not marked as WIP as I can see. Can you elaborate whether you are going to merge this PR with a disabled test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request has no status/code-review
label as well, but you're still reviewing it. Of course I have no plans to merge disabled unit test as that will cause test suit regression, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the clarification. BTW there is a bot which puts label status/code-review
on PRs.
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely +1
Please finalize it with documentation:
#7869
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4393/ |
ci-test |
ci-test build report: |
Related issue: #7582
Overview
Added basic workspace agent functionality for interaction with language servers that are run in a separate container. In order to use remote language server instance it must support either TCP connection or
stdio
in conjunction withsocat
utility. The language server launchers are dynamically created and in general it is enough to make edits to workspace configuration and use a docker image with language server instance configured and running. However there is a limitation of supported languages: #7790.Video of the use case is here: https://youtu.be/s0CCGfA8-1M
Workspace configuration
There are several basic things that you must do to use remote language server:
typescript-ls-machine
in examplels
in examplels
type of serversSerialized string is a serialized stringified that represents a
LanguageServerDescription
class.Workspace configuration example:
Language server config example:
Dockerfile example:
Api changes
LanguageServerLauncher
has new methodisLocal()
that returnstrue
by default and indicates if language server will be launched locally or remotely.RemoteLsLauncherProvider
that is to process workspace configuration and provide all configured remote language server launchers.Guice changes
LsRemoteModule
to aggregate all remote language server specific configuration classes and implementations.Many thanks to @garagatyi and @eivantsov for assistance on docker related stuff! :-)