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

#150 #141 Rework dependency injection architecture #127

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented 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
Copy link
Contributor Author

tortmayr commented Sep 8, 2021

Here is a little illustration of the new injection approach:
Instead of using one global injector per application the Injection-context gets split into a a global server injector per application and dedicated child injectors for each client session. The server injector only provides infrastructure and networking classes. Diagram language and session specific bindings are contributed by the client session specific injector.
Instead of using one global injector per application connection, t Instead use a global server injector and dedicated child injectors for each client session.
The server injector is created when the client application initially connects. When the server receives a new InitializeClientSession request it delegates to the ClientSessionManager who than create a new ClientSession. The client session itself contains the client session injector and exposes the ActionDispatcher so that the GLSPServer can delegate incoming action messages to the action dispatcher of the corresponding client session.
Screenshot from 2021-09-08 17-20-09

@tortmayr tortmayr self-assigned this Sep 8, 2021
Copy link
Member

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent job! Code looks great and clean, everything still works as expected.
I'm looking forward to how we can now make the handlers more flexible in terms of API!

@Target({ TYPE, METHOD })
@Retention(RUNTIME)
@ScopeAnnotation
public @interface DiagramGlobal {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename that to DiagramGlobalSingleton?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe DiagramTypedSingleton? To make it more clear that these scope provides Singletons in the context of the diagram type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, I'd prefer DiagramTypeSingleton a bit over DiagramTypedSingleton or DiagramGlobalSingleton.

tortmayr and others added 2 commits September 16, 2021 11:56
#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 tortmayr changed the title [Proposal] #150 #141 Rework dependency injection architecture #150 #141 Rework dependency injection architecture Sep 16, 2021
@tortmayr
Copy link
Contributor Author

Thanks for your Review Philip 👍 . I have addressed your feedback and also rebased the change to resolve conflicts.
As mentioned inline the @Inject annotations needs to present even if additional custom annotations are used. I reverted the corresponding change that you did in a0d82f2.

Copy link
Member

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks great!

@tortmayr tortmayr merged commit 869e575 into master Oct 5, 2021
@tortmayr tortmayr deleted the tortmayr/di-rework branch October 5, 2021 21:40
tortmayr added a commit that referenced this pull request Oct 19, 2021
With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers
can simply directly inject the model state if the want to use it.  Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Fixes eclipse-glsp/glsp/issues/120)

- Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler`
- Remove `Handler` super interface because it doesn't provide any additional value
- Add documentation for affected API.
- Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler`
- Deprecate `BasicOperationHandler` & `BasicActionHandler`
tortmayr added a commit that referenced this pull request Oct 19, 2021
With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers
can simply directly inject the model state if the want to use it.  Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120)

- Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler`
- Remove `Handler` super interface because it doesn't provide any additional value
- Add documentation for affected API.
- Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler`
- Deprecate `BasicOperationHandler` & `BasicActionHandler` & `BasicCreateOperationHandler`
- Replace usage of `BasicActionHandler` with `DefaultActionHandler`
- Replace usage of `BasicOperationHandler'  with `DefaultActionHandler`
- Replace usage of `BasicCreateOperationhandler'  with `DefaultOperationHandler` & `DefaultCreateOperationHandler`

Fixes eclipse-glsp/glsp/issues/425
tortmayr added a commit that referenced this pull request Oct 25, 2021
With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers
can simply directly inject the model state if the want to use it.  Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120)

- Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler`
- Remove `Handler` super interface because it doesn't provide any additional value
- Add documentation for affected API.
- Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler`
- Deprecate `BasicOperationHandler` & `BasicActionHandler` & `BasicCreateOperationHandler`
- Replace usage of `BasicActionHandler` with `DefaultActionHandler`
- Replace usage of `BasicOperationHandler'  with `DefaultActionHandler`
- Replace usage of `BasicCreateOperationhandler'  with `DefaultOperationHandler` & `DefaultCreateOperationHandler`

Fixes eclipse-glsp/glsp/issues/425
tortmayr added a commit that referenced this pull request Oct 26, 2021
* #425#120 Rework Actionhandler/Operationhandler API

With the DI rework #127 its no longer necessary to pass the model state object as part of the execute method. instead action/operation handlers
can simply directly inject the model state if the want to use it.  Since action & operation handlers are now client session scoped an arbitary state object can be injected and used for execution. (Part of eclipse-glsp/glsp/issues/120)

- Remove the modelstate argument from the execute() method of `ActionHandler` and `OperationHandler`
- Remove `Handler` super interface because it doesn't provide any additional value
- Add documentation for affected API.
- Introduce new `DefaultActionHandler` & `DefaultOperationHandler` that serve as replacement for the now deprecated `BasicActionHandler` & `BasicOperationHandler`
- Deprecate `BasicOperationHandler` & `BasicActionHandler` & `BasicCreateOperationHandler`
- Replace usage of `BasicActionHandler` with `DefaultActionHandler`
- Replace usage of `BasicOperationHandler'  with `DefaultActionHandler`
- Replace usage of `BasicCreateOperationhandler'  with `DefaultOperationHandler` & `DefaultCreateOperationHandler`

Fixes eclipse-glsp/glsp/issues/425


* Remove GModelState from interfaces as it should now be injected instead

This simplifies the use of custom model state classes because
implementations can now inject their concrete custom class instead of
the generic GModelState that they'd have to cast again.
Also several implementations often don't need the model state making the
method parameter superfluous.

Fixes eclipse-glsp/glsp/issues/120


Co-authored-by: Philip Langer <planger@eclipsesource.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants