feat: add request/response support to server ActionDispatcher#131
feat: add request/response support to server ActionDispatcher#131martin-fleck-at wants to merge 2 commits intomainfrom
Conversation
Enable the server to send requests to the client and await responses, and to issue requests handled locally by server-side handlers. Complements the client-side changes in glsp-client. - Add `request()` and `requestUntil()` to ActionDispatcher - Intercept responses in `dispatch()` and `doDispatch()` to resolve pending requests - Translate `RejectAction` responses to promise rejections - Bypass action queue for nested requests to prevent deadlocks - Tighten `shouldForwardToClient()` to use `hasValidResponseId()` - Add async `handleClientRequest()` in `DefaultGLSPServer.process()` - Add tests for request/response, deadlocks, timeouts, late responses, and dispose cleanup Relates to eclipse-glsp/glsp#607
tortmayr
left a comment
tortmayr
left a comment
There was a problem hiding this comment.
Here are my review comments after a first high level review and quick testing.
One additional issue:
We currently have no actual server side request in the example code i.e. no way to test the feature e2e or manually. We currently solely rely on unit tests.
Maybe it would be a good to exetend the workflow example with handler that actually uses the new request handling. Could be something simlple like retrieving the current editor context from the client and logging it.
| * _not_ passed to the registered action handlers. Instead, it is the responsibility of the | ||
| * caller of this method to handle the response properly. | ||
| * | ||
| * If the request's `kind` is registered in `ClientActionKinds`, it is forwarded to the client |
There was a problem hiding this comment.
IMO I think this is the part that needs some testing and hardening.
For the request-response feature we work on the implicit assumption that the handling side has exactly one action handler that returns the corresponding response. Multiple handlers for the action kind can be registered, but they either need to be void or return other action/command types.
We currently have no way to verify that assumption. So what happens if we have two registered response handlers for a request.
- Which one wins?
- What happens to the response action of the second handler? Is it rejected, dispatched normally, erroneous at the receiving side etc.
Similar to that, what happens if both the client and server have registered a response handler for the request action.
This is implicit a flawed design since we have this only one response handler restriction.
But are these cases handled gracefully?
Which side wins. does this cause errors on the other side, is there logging/usable output for the adopter if the run into this case etc.
Maybe we should continue the discussion to this topic in a different channel (i.e. face-to-face meeting)
There was a problem hiding this comment.
I documented this assumption of only the first response handler resolving the request, any other responses (late, second responses or responses without request) are simply dispatched as normal actions, as they are on the client.
| } | ||
|
|
||
| // Dont queue actions that are just delegated to the client | ||
| if (this.clientActionForwarder.shouldForwardToClient(action)) { |
There was a problem hiding this comment.
Not directly related to this PR, but maybe an issue that we should track in a ticket:
This queue skipping for client actions could be problematic if the server has also registered an handler for this action kind.
In that case, the server handled actions escapes the sequential order is dispatched directly before any other queued server actions.
There was a problem hiding this comment.
I adapted the code now that client actions do not skip the queue automatically but instead we have a proper dispatchDirectly to dispatch actions without the queue, e.g., for progress reporting.
| } else { | ||
| deferred.resolve(action); | ||
| } | ||
| if (postUpdateActions.length > 0) { |
There was a problem hiding this comment.
Shouldn't we handle the case of rejeced SetModel UpdateModel action here with an explicit error.
i.e. if the action was rejected but we have postUpdateModel actions we are in a potentially fatal state.
The core model update went wrong and we should inform the user.
There was a problem hiding this comment.
If we run into a project, we should have a RejectAction anyway and the post update actions are empty.
| ? await clientSession.actionDispatcher.requestUntil(action, action.timeout, true) | ||
| : await clientSession.actionDispatcher.request(action); | ||
| if (response) { | ||
| this.sendToClient({ clientId, action: response }); |
There was a problem hiding this comment.
Similar to client side. Maybe extract this into submethod for more user control.
- Document single-handler assumption and late/extra response behavior on request() - Shorten RejectAction/postUpdateQueue comment - Add dispatchDirectly() to bypass the action queue for actions that need immediate processing (e.g. progress notifications) - Use dispatchDirectly() for handler responses and nested requests - Remove client-action queue bypass from dispatch(); all actions are now enqueued to preserve sequential ordering - Extract sendResponseToClient() hook in DefaultGLSPServer
|
@tortmayr I updated the PR and now we have a more explicit handling of actions:
As for the example, there really is no good place in the workflow example to do something semantically useful. However, with the upcoming MCP feature, we will need it for the tool functions and there the usage will be well-tested. Is that enough? |
2 participants
What it does
Enable the server to send requests to the client and await responses, and to issue requests handled locally by server-side handlers. Complements the client-side changes in glsp-client.
request()andrequestUntil()to ActionDispatcherdispatch()anddoDispatch()to resolve pending requestsRejectActionresponses to promise rejectionsshouldForwardToClient()to usehasValidResponseId()handleClientRequest()inDefaultGLSPServer.process()Relates to eclipse-glsp/glsp#607
How to test
Follow-ups
Changelog
It adds two methods to the server-side action dispatcher. We do have default implementations but if adopters have their own implementation, they will be missing those methods.
Note: Will break until the protocol changes in eclipse-glsp/glsp-client#480 are merged.