#96 #94 Cleanup and refactor client-server communication #73
Conversation
- Refactor client-server communication into a base protocol and a jsonrpc specific implementation (#94) - Cleanup protocol and implement (previously unused) shutdown method - Add ClientSessionManager to track lifecycle of GLSP client connections (#96) - Add listener-mechanism to react to lifecycle changes (used in DefaultModelStateProvider) - Remove (now obsolete) GLSPClientProxyProvider - Fix wrong version of guava in org.eclipse.glsp.graph pom.xml - Add unique applicationId to InitializeParameters Part of: - eclipse-glsp/glsp/issues/94 - eclipse-glsp/glsp/issues/96
-Update dependencies to conform to sprotty 0.9.0 and Theia 1.3.0 - Adapt code to confrom to client/server changes - Change argument for passing the server port to "WF_GLSP" Requires: - eclipse-glsp/glsp-server/pull/73 - eclipse-glsp/glsp-theia-integration/pull/44 Part of: - eclipse-glsp/glsp/issues/104 - eclipse-glsp/glsp/issues/94 - eclipse-glsp/glsp/issues/96 - eclipse-glsp/glsp/issues/105
|
@CamilleLetavernier Could you have a look at the |
|
|
||
| boolean removeListener(ClientSessionListener listener); | ||
|
|
||
| Optional<GLSPClient> resolve(String clientId); |
There was a problem hiding this comment.
Is this reliable? If I understand correctly, 1 GLSPClient corresponds to one "tab" in a browser, and 1 client ID corresponds to one "Widget" in a tab. So if we have 2 different GLSPClients, they may use the same clientId (Typically, xxx-diagram_0_widget). Since (AFAIU) the ClientSessionManager is shared across multiple modules, we can't assume that clientId will be unique ids (They are only unique for 1 GLSPClient / injection Module)
There was a problem hiding this comment.
Each client application ("tab") now generates a unique applicaitonId that is also part of the clientId/widgetId.
(https://github.com/eclipse-glsp/glsp-theia-integration/blob/6fa15d9a0c374af2f5aa641884d880d84b70b08a/src/browser/diagram/glsp-diagram-manager.ts#L75)
This ensures that client ids are unique even when multiple actual clients are connected.
There was a problem hiding this comment.
Ah, great! Then it looks good to me :)
|
|
||
| private final Set<GLSPClient> clients = new HashSet<>(); | ||
| private final Set<ClientSessionListener> listeners = new LinkedHashSet<>(); | ||
| private final Map<String, GLSPClient> clientSessions = new HashMap<>(); |
There was a problem hiding this comment.
I think this doesn't work with the way clientIds are currently generated
There was a problem hiding this comment.
Works because I adapted the way that clientIds are generated. They are now unique for each actual client/application
There was a problem hiding this comment.
I didn't test the changes; but the ClientSessionManager API looks good to me. I'll have to test that in real application to see if notifications are properly sent in all cases (Especially, I didn't see how/if the server can trigger disconnectClient when the connection is lost. I'm not sure that's even desired; e.g. if the client can reconnect using the same app/client Id without reloading the tab)
There was a problem hiding this comment.
Great work, thanks a lot!
I've appended a few minor changes in a separate commit for your review. Other than that, this looks great!
Just a general remark regarding copyrights, please keep the original where applicable but just update the year (e.g. original 2019 --> now 2019 - 2020).
| /******************************************************************************* | ||
| * Copyright (c) 2019 EclipseSource and others. | ||
| /******************************************************************************** | ||
| * Copyright (c) 2020 EclipseSource and others. |
There was a problem hiding this comment.
I think this actually should then be 2019 - 2020 ....
There was a problem hiding this comment.
In general you are right. But you picked a bad example. The DisposeClientAction was introduced with this commit so the copyright year should be 2020. This is displayed weirdly because Github is diffing GLSPClientAware <-> DisposeClientAction
There was a problem hiding this comment.
I did not touch any existing copyright headers in this PR
plugins/org.eclipse.glsp.api/src/org/eclipse/glsp/api/protocol/ClientSessionListener.java
Outdated
Show resolved
Hide resolved
...clipse.glsp.server/src/org/eclipse/glsp/server/actionhandler/DisposeClientActionHandler.java
Outdated
Show resolved
Hide resolved
* #105 #104 #96 #94 #39 Refactor client-server communication -Update dependencies to conform to sprotty 0.9.0 and Theia 1.3.0 - Adapt code to confrom to client/server changes - Change argument for passing the server port to "WF_GLSP" Requires: - eclipse-glsp/glsp-server/pull/73 - eclipse-glsp/glsp-theia-integration/pull/44 Part of: - eclipse-glsp/glsp/issues/104 - eclipse-glsp/glsp/issues/94 - eclipse-glsp/glsp/issues/96 - eclipse-glsp/glsp/issues/105 * Update launch config to changed env args * Adapt copyright headers * Update versions Co-authored-by: Philip Langer <planger@eclipsesource.com>
DefaultModelStateProvider)
Part of: