Fix minor typos and clarify non-blocking handler guidance#975
Fix minor typos and clarify non-blocking handler guidance#975Kaike-blanck wants to merge 3 commits into
Conversation
pisv
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Returning a CompletableFuture as suggested in the clarification is not possible for notification methods. In general, I think that adding a code example would be the best way to clarify the non-blocking handler guidance.
| When implementing the handlers for requests or notifications, you need to be aware that the calling thread is the thread that reads and dispatches incoming messages. Therefore, blocking it may result in reduced throughput or even a deadlock (https://github.com/eclipse-lsp4j/lsp4j/issues/775). As a general rule, message handlers should be implemented in a non-blocking, asynchronous way. | ||
|
|
||
| For long-running work, prefer returning a `CompletableFuture` and performing the actual computation outside of the message dispatching thread. This keeps the JSON-RPC message processor responsive while the request is being handled asynchronously. |
There was a problem hiding this comment.
I think that using a code example could be the best way to clarify it, e.g. based on the actual case from #775:
| When implementing the handlers for requests or notifications, you need to be aware that the calling thread is the thread that reads and dispatches incoming messages. Therefore, blocking it may result in reduced throughput or even a deadlock (https://github.com/eclipse-lsp4j/lsp4j/issues/775). As a general rule, message handlers should be implemented in a non-blocking, asynchronous way. | |
| For long-running work, prefer returning a `CompletableFuture` and performing the actual computation outside of the message dispatching thread. This keeps the JSON-RPC message processor responsive while the request is being handled asynchronously. | |
| When implementing the handlers for requests or notifications, you need to be aware that the calling thread is the thread that reads and dispatches incoming messages. Therefore, blocking it may result in reduced throughput or even a deadlock (https://github.com/eclipse-lsp4j/lsp4j/issues/775). As a general rule, message handlers should be implemented in a non-blocking, asynchronous way. For example, instead of | |
| ```java | |
| @Override | |
| public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) { | |
| build(); | |
| } | |
| ``` | |
| use | |
| ```java | |
| @Override | |
| public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) { | |
| CompletableFuture.runAsync(() -> { | |
| build(); | |
| }); | |
| } | |
| ``` |
A separate example along these lines could also be added for a request handler, if necessary.
|
Also, @RyanBBlanck as the author of the proposed changes needs to sign ECA before this contribution can be accepted. Be sure to use the same email address when you register for the Eclipse Foundation account that you used for your commits. Thanks, and sorry for the hindrance! |
Summary
This PR includes three small documentation and cleanup changes:
Fixes a typo in the warning message for unknown message types in RemoteEndpoint.
Fixes a typo in the Javadoc of TracingMessageConsumer.
Clarifies the recommendation for request and notification handlers to avoid blocking the message dispatching thread.
Motivation
These changes are minor, but they improve readability and documentation clarity. The added note in the documentation makes the existing recommendation about non-blocking handlers more explicit by suggesting the use of CompletableFuture for long-running work.
Impact
This PR does not change runtime behavior or public APIs. The changes are limited to a log message typo, Javadoc text, and documentation.