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

[Server] Consider splitting network-related classes from language-related classes, in the modules/injector #150

Closed
CamilleLetavernier opened this issue Nov 26, 2020 · 3 comments · Fixed by eclipse-glsp/glsp-server#127
Assignees
Projects

Comments

@CamilleLetavernier
Copy link
Member

Currently, we use the same injector for network-related classes (GLSPServer, GLSPClient) and for language-related classes (ActionDispatcher, ActionHandlers...)

While this works fine in the main use case (1 cloud-server, multiple browser-clients), it makes it difficult to e.g. embed GLSP in an IDE. The reason is that we have:

[Note: GLSPServer, below, corresponds to the server-side instance of the client-server relationship. It doesn't mean we start several instances of the web-server. It corresponds to the org.eclipse.glsp.server.protocol.GLSPServer interface in the code]

  • 1 GLSPServer per GLSPClient
  • 1 GLSPServer instance per Injector (i.e. the GLSPServer is a Singleton in the Injector)

This means that multiple client connections (GLSPClient/GLSPServer pair) can't share the same injector (ActionHandlers, ...)

In the standard Cloud scenario, we have the following:

  • 1 Browser Tab = 1 GLSPClient/GLSPServer pair = 1 Network connection
  • 1 Browser Tab may open multiple widgets (with a locally unique clientId). All widgets then share the same GLSPClient/GLSPServer connection, and the same language-injector (Same handlers, same ActionDispatcher)

However, when integrating a GLSP Editor in e.g. the Eclipse IDE, we can't apply this pattern, because each editor opens its own Browser widget, thus needs its own GLSPServer/GLSPClient connection. This means we need the following:

  • 1 EditorPart = 1 "BrowserTab" = 1 Network connection (GLSPClient/GLSPServer pair)
  • 1 EditorPart opens a single widget (with an IDE-unique clientId, although that doesn't matter)
  • Since GLSPServer is unique inside of the Injector, we need 1 Injector per Widget. This means we cannot (easily) reuse language-related instances (ActionDispatcher, ActionHandler... and shared resourceSet for example).

In order to be more flexible when integrating a GLSP Server into an application (as opposed to a standalone web-server), we should split the Network-related classes/modules from the language-related modules. Then we may use a shared language-module for multiple Network-modules, when that makes sense; without being stuck with Network-Singletons.

A consequence would be that we can't rely on the injected GLSPClient anymore (e.g. for lifecycle-related operations, or simply for sending messages back to the client, via the ClientActionHandler). So we need to abstract the GLSPServer/GLSPClient. Since clientIds are not universally unique (They're only unique for one GLSPClient/GLSPServer connection), we need to be careful about this.

@CamilleLetavernier
Copy link
Member Author

For reference, this is the original issue: #149

At the moment, to solve this, we need to use unique injectors

@tortmayr tortmayr added this to New in GLSP kanban via automation Dec 16, 2020
@planger planger moved this from New to Backlog in GLSP kanban Dec 17, 2020
@planger planger added this to the 1.0.0 Graduation Release milestone Jul 9, 2021
@tortmayr tortmayr moved this from Backlog to In progress in GLSP kanban Jul 10, 2021
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 4, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 5, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 8, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .
- Use the new approach of injecting optionals (provided by Guice 5) instead of binding NullImpls.

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes
- Follow Guice best practices and document all public bindings of a module

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 8, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .
- Use the new approach of injecting optionals (provided by Guice 5) instead of binding NullImpls.

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes
- Follow Guice best practices and document all public bindings of a module

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 8, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .
- Use the new approach of injecting optionals (provided by Guice 5) instead of binding NullImpls.

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes
- Follow Guice best practices and document all public bindings of a module

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Sep 16, 2021
#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .
- Use the new approach of injecting optionals (provided by Guice 5) instead of binding NullImpls.

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes
- Follow Guice best practices and document all public bindings of a module

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141
GLSP kanban automation moved this from In progress to Done Oct 5, 2021
tortmayr added a commit to eclipse-glsp/glsp-server that referenced this issue Oct 5, 2021
* [WIP] #150 #141 Rework dependency injection architecture

#150 Rework DI architecture
No longer use one global injector per application connection. Instead use a global server injector and dedicated child injectors for each client session. This removes a couple of flaws/downsides we had with the current approach. To achieve this the `GLSPModule` has been split into two separate modules. The `ServerModule` provides the bindings for base infrastructure & networking (used for creating the server injector) while `DiagramModules` provide language & session specific bindings (used for creating the session specific child injectors).

The main benefits are the following:
- Each session now uses its own `ActionDispatcher` this means action messages intended for different client sessions can now be handled in parallel. Previously they were processed one after the other essentially creating a bottleneck.
- One `GLSPServer` can now handler multiple different diagram implementations. The diagram language specific bindings are provided with a `DiagramModule`. When creating a new client session injector the corresponding `DiagramModule` is identified via the given diagram type.  This can for instance be used in UML-GLSP to provide all different diagram types (class,package,state machine etc.) with one server.
- Apart from the initial "process" method it is no longer necessary to keep track of the diagramid/client session id. Each session has its own instances of action handlers, operation handlers, model state, diagram configuration etc. and the clientId no longer needs to be passed as method argument. In addition, provider classes with the purpose of mapping a class instance to the correct digramId like `ModelStateProvider` or the `DiagramConfigurationRegistry` are now obsolete and have been removed.
- It is now possible to create stateful action/operation handlers. Previously handlers had to be stateless because they were reused for all client sessions. Now  each session has its own set own set of handlers .
- Use the new approach of injecting optionals (provided by Guice 5) instead of binding NullImpls.

Note that this PR only provides the DI architecture rework. Existing API whose functionality does not break after the rework as not been touched yet even tough there a various aspects of  the existing API that can now be simplified/improved. For instance, with the new approach the Action/Operation Handler API can be simplified. It is no longer necessary to pass the model state argument as part of the interface methods (see also #120). I have identified some major aspects of the API that can be improved (corresponding issues will be created after this PR is merged).

#141 Refactor `ClientSessionListener` API
- Extract listener methods for server connection from `ClientSessionListener` interface into own `ServerConnectionListener` interface. Server connection listeners are handled directly by the `GLSPServer`.
- It is now possible to define a set of clientIds for which a listener should be registered when adding a new `ClientSessionListener` to the 'ClientSessionManager`.
- The  `DefaultGLSPClientSessionManager` now autodisposes all `ClientSessionListeners` for the corresponding clientSessionId when a session is disposed.
-  Improved null-checks: The client session manager now checks for disposed listeners (i.e. null values in the map) and removes them before notifying (i.e invoking a listener method) them.

Miscellaneous
- Reduce public API exposure. Interface implementations that typically are not expected to be customized by implementing projects haven been moved to the `internal` API.
- Improve documentation. Add javadoc for all newly added interfaces and relevant public classes. Also improve documentation of some existing components.
- Merge `GLSPServerStatusAction` and `ServerStatusAction`
- Introduce `ModuleUtil`  class with provides the functionality to mixin modules similar to how it is done in Xtext.
- Remove uncessary  `.gitkeep` file.
- Rename `DefaultGLSPServerLauncher` to `SocketGLSPServerLauncher`
- Rename `ClientOptions` to `ClientOptionsUtil` to conform to the standard naming scheme for util classes
- Follow Guice best practices and document all public bindings of a module

Fixes eclipse-glsp/glsp/issues/150
Fixes eclipse-glsp/glsp/issues/141

* Remove clientId argument where obsolete

* Address review feedback

Co-authored-by: Philip Langer <planger@eclipsesource.com>
@ivy-cst
Copy link
Contributor

ivy-cst commented Oct 11, 2021

Very cool, thanks @tortmayr. If I am right also the server part of the glsp-eclipse-integration needs this change. Do you have any plan to fix that or an Issue for that? Because we like to go to the latest glsp-server but we also have the glsp-eclipse-integration in our workspace. Thanks for your answer and your work.

@tortmayr
Copy link
Contributor

@ivy-cst Yes you are right. The glsp-eclipse-integration also needs this change and some refactoring of the code base is required. I opened #418 to track this. I already have a local change prepared for this and just need to do some testing before I open the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
GLSP kanban
  
Done
4 participants