Experimental client-side command executions #596
Conversation
Signed-off-by: Kris De Volder <[email protected]>
kdvolder
force-pushed
the
execute-client-command
branch
from
6b8098a to
f735011
Compare
|
|
||
| public interface ExecuteCommandProposedClient { | ||
|
|
||
| @JsonRequest("workspace/executeCommand") |
There was a problem hiding this comment.
there's already a "workspace/executeCommand" command on the server side, so I suggest using a different name
There was a problem hiding this comment.
I thought it logical to use the same name as it actually does exactly the same thing just the message are flowing in the opposite direction. But sure... could change it to 'workspace/executeClientCommand' or maybe 'client/executeCommand'? What is more logical?
There was a problem hiding this comment.
either one is fine I guess, I have no strong opinion for one or the other
Change it to `workspace/executeClientCommand` to distinguish it from existing `workspace/executeCommand` on the server side.
Follow through on renaming protocol message "workspace/executeCommand". Java methods involved in handling this message now also renamed.
|
So @tsmaeder and I agreed we'll go on with the approach in this PR rather than #575. Can you please add some tests before we can proceed? Take a look at https://github.com/eclipse/eclipse.jdt.ls/pull/575/files#diff-1514fea0a7698e4d85f57581474c7cd7R37 for inspiration. |
| public interface ExecuteCommandProposedClient { | ||
|
|
||
| @JsonRequest("workspace/executeClientCommand") | ||
| CompletableFuture<Object> executeClientCommand(ExecuteCommandParams params); |
There was a problem hiding this comment.
Do we actually need to wait for the client to respond?
There was a problem hiding this comment.
I think it is a good idea yes. One possible and useful scenario is an error results because the command was deregistered. If command is a kind of 'callback' as in our case, then this may be a good way to trigger cleanup of the callback table.
I've experimented with a more 'proper' cleanup. But as this tends to happen around the same time as server is in the process of shutting down... its not very reliable (messages no longer able to send etc.).
Anyhow... callers wanting to catch errors and responding to them seems to be a good enough reason. And I could also imagine that the command could return useful information (not in our use case, but if it used in a more 'request -> response fashion, rather than a callback it could be).
| @@ -56,6 +61,10 @@ public JavaClientConnection(JavaLanguageClient client) { | |||
| logHandler.install(this); | |||
| } | |||
|
|
|||
| public Object executeClientCommand(String id, Object... params) { | |||
| return this.client.executeClientCommand(new ExecuteCommandParams(id, ImmutableList.copyOf(params))).join(); | |||
There was a problem hiding this comment.
should probably add a timeout
There was a problem hiding this comment.
That raises the question... how long should the timeout be? Or should we change this api and return the CompletableFuture? Then the caller can decide how long they want to wait... or if they want to continue with an async composition instead.
There was a problem hiding this comment.
Hmmm... returning CompletableFuture seems a bit 'out of style' with the rest of the api in the same class. I called join because I as trying to follow similar pattern as other stuff in the same class. E.g. like here:
Signed-off-by: Kris De Volder <[email protected]>
|
@fbricon Added some tests and also an alternate api that allows passing a Duration for timeout. |
| * http://www.eclipse.org/legal/epl-v10.html | ||
| * | ||
| * Contributors: | ||
| * Red Hat Inc. - initial API and implementation |
| * http://www.eclipse.org/legal/epl-v10.html | ||
| * | ||
| * Contributors: | ||
| * Red Hat Inc. - initial API and implementation |
There was a problem hiding this comment.
Ah... yeah. The headers were all added automagically by Eclipse (you must have configured it that way in the projects). And I didn't pay attention so I didn't notice.
I'll make pass through all the copyright headers of all the changed files.
Experimental execute client-side command protocol extension Signed-off-by: Kris De Volder <[email protected]>
This is one half of a proposed protocol extension that allows language server to execute commands on the server.
It requires another PR to implement support for handling the new
workspace/executeCommandmessage on the client side.This fits in with in the discussion at #595