Complete pending server-initiated request promises on default executor

[ferdinand-beyer]

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.

[mainej]

Good catch. See comments for a few questions, but otherwise, looks good to me!

[mainej]

Is it possible for users to be using the default executor as well? And are we sure that our thread isn't in the default executor?

[ferdinand-beyer]

Is it possible for users to be using the default executor as well?

Not sure if I understand what you mean. Of course they can use the default executor, but there are caveats:

• they would need to remember to pass it to every p/then (and friends) on the returned promise
• last time I checked there was no p/catch that accepts an executor

It does not matter which thread executes the original send-request call, only which thread completes the promise.

And are we sure that our thread isn't in the default executor?

Yes, as we are called from the pipeline, which is a core.async/go-loop. Our thread is therefore in core.async's fixed-size dispatch thread-pool, that executes all go blocks.

[mainej]

Our thread is therefore in core.async's fixed-size dispatch thread-pool,

Got it. That makes sense.

Not sure if I understand what you mean

I guess my question was whether we're causing problems for our users, by having them to execute their callbacks in the default executor (the common fork-join pool). If they already have a lot of stuff going on in that executor, could it lead to other issues for them? If so, what would we do? Is this materially better or worse for them than what we had before? I lack the familiarity with the underlying tools to answer those questions.

[ferdinand-beyer]

I think the current behaviour is definitely worse, since right now we run code that can potentially block in a thread pool that is not designed for this. In general, I think go blocks are really only meant to move things between channels, everything that requires non-trivial computation or even I/O should be done in threads.

But sure, it would make sense to allow users to specify the executor for responses. I will update this PR to make it configurable.

[ericdallo]

LMK when you 2 think it's ready to merge/release

[ferdinand-beyer]

I think this is mergeable. What do you think about letting users override the executor though?

[ericdallo]

What do you think about letting users override the executor though?

TBH I like it to give a option, if people want to do that, they probably know what they are doing :)

[ferdinand-beyer]

@mainej @ericdallo -- I've added a :response-executor option, which defaults to :default (= ForkJoinPool/commonPool). Users can:

• specify :current-thread for the old behaviour
• use :vthread if they running on a recent JVM
• specify their own executor instance

I've also added tests for that.

[ericdallo]

Looks great!

[ericdallo]

@ferdinand-beyer could you an entry on CHANGELOG.md please?

[ferdinand-beyer]

Done. Also updated the README.

I sneaked in a change to deps.edn. I hope that this is OK.

The reason is that with my tooling (Calva), I'd like to have the test directory in my classpath, but without running the tests. I normally use two aliases:

• :test for configuring the test classpath with :extra-paths and :extra-deps, enabled during development
• :test/run or :kaocha to use the test runner of my choice

Did not want to mess with that here, keeping clojure -M:test to run the tests, so I added a new :dev alias for my needs.

[ericdallo]

That's ok add that alias!
Thank you!

[ericdallo]

Will wait for #44 merge to do a release

[ferdinand-beyer]

Will wait for #44 merge to do a release

Makes sense! Will update that one to resolve the conflict and add the :response-executor to the chan-server docstring.