Skip to content

Complete pending server-initiated request promises on default executor #43

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 10, 2024
Merged

Complete pending server-initiated request promises on default executor #43

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fe73a2c
Fix `.cljfmt.edn`
ferdinand-beyer Jul 19, 2024
85d1a8a
Complete pending request promise on default executor
ferdinand-beyer Jul 19, 2024
6add493
Improve comment
ferdinand-beyer Jul 25, 2024
58139e1
Support user-defined response executor
ferdinand-beyer Aug 6, 2024
6aaf086
Simplify test (go blocks return channels)
ferdinand-beyer Aug 6, 2024
3de5ded
Update readme and changelog
ferdinand-beyer Aug 9, 2024
cb24771
Add `:dev` alias
ferdinand-beyer Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .cljfmt.edn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
:remove-consecutive-blank-lines? true
:insert-missing-whitespace? true
:align-associative? false
:indents {#re "^(?!catch-kondo-errors).*" [[:block 0]]
catch-kondo-errors [[:inner 0]]}
:extra-indents {#re "^(?!catch-kondo-errors).*" [[:block 0]]
catch-kondo-errors [[:inner 0]]}
:test-code
(comment
(:require
Expand Down
15 changes: 11 additions & 4 deletions src/lsp4clj/server.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
;; client. This cannot be `(-> (p/deferred) (p/catch))` because that returns
;; a promise which, when cancelled, does nothing because there's no
;; exception handler chained onto it. Instead, we must cancel the
;; `(p/deffered)` promise itself.
;; `(p/deferred)` promise itself.
(p/catch p CancellationException
(fn [_]
(protocols.endpoint/send-notification server "$/cancelRequest" {:id id})))
Expand Down Expand Up @@ -351,9 +351,16 @@
(if-let [{:keys [p started] :as req} (get pending-requests id)]
(do
(trace this trace/received-response req resp started now)
(if error
(p/reject! p (ex-info "Received error response" resp))
(p/resolve! p result)))
;; Note that we are called from the server's pipeline, a core.async
;; go-loop, and therefore must not block. Callbacks of the pending
;; request's promise will be executed in the completing thread,
;; which should not be our thread. This is very easy for users to
;; miss, therefore we complete the promise on the default executor.
(p/thread-call :default
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ferdinand-beyer ferdinand-beyer Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

(fn []
(if error
(p/reject! p (ex-info "Received error response" resp))
(p/resolve! p result)))))
(trace this trace/received-unmatched-response resp now)))
(catch Throwable e
(log-error-receiving this e resp))))
Expand Down