lsp4clj icon indicating copy to clipboard operation
lsp4clj copied to clipboard

Complete pending server-initiated request promises on default executor

Open ferdinand-beyer opened this issue 1 year ago • 1 comments
trafficstars

There's a subtle detail on Promesa promises (CompletableFuture): Unless an executor is specified, callbacks will be executed on the thread that completed the promise.

For lsp4clj, this means that users need to be careful when using send-request and coercing the result to a Promesa promise: The code run in handlers such as p/then or p/catch will end up running in a core.async dispatch thread, as the promise is completed from within a go-loop.

Since go blocks are executed in a fixed-size thread pool, one should not block in go blocks. Consequently, users should not block in code that handles the send-request promise, unless they explicitly move it to a different executor.

This is easy to get wrong, especially with Promesa's convenience macros like p/let, which will coerce the PendingRequest to a promise without a dedicated executor. Furthermore, Promesa does not expose CompletionStage.exceptionallyAsync(), i.e. it does not support specifying an executor with p/catch, therefore it is not trivial to ensure all continuation happens in another thread.

This PR attempts to improve this by completing the promise in a thread from the :default executor ((ForkJoinPool/commonPool)), instead of the calling thread.


Some caveats to this approach:

  • There's some overhead even when the promise does not have any continuation handlers (apart from the CancellationException we handle ourselves, which, since it blocks on a send-notification call, should also run on a suitable executor)
  • We don't allow users to customise the executor, but use promesa's default, which is (ForkJoinPool/commonPool). Users might prefer to use a virtual thread executor instead. We could add a :response-executor option for this?

The change to .cljfmt.edn is unrelated, but required for more recent version of cljfmt, as used by Calva, for example. This could be moved to a different PR of course.

ferdinand-beyer avatar Jul 19 '24 12:07 ferdinand-beyer