-
-
Notifications
You must be signed in to change notification settings - Fork 68
refactor: make progress tracking better follow LSP semantics #260
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.
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
base: main
Are you sure you want to change the base?
Conversation
* Overhaul `Expert.Project.Progress` with a new progress reporting API. * Progress reporting only informs users of long-running work and doesn’t drive behavior. New interface accordingly KISS. * Removed releveant `__using__` macros in favor of simple `alias` + function calls. * Use plain tuples instead of `defrecord` for progress report messages between the server node and engine node. * Debounce namespace build logs to minimize spam.
* Store the manager node name on startup in the engine node. This fixes up some tests that were faltering due to the introduced rpc calls. This is more stable anyway, imo. * give project compilation more generous timeouts in testing (5s instead of 100ms)
| {:ok, response} <- Expert.Protocol.Convert.to_lsp(response) do | ||
| Task.Supervisor.start_child(:expert_task_queue, fn -> | ||
| # dirty sleep to allow initialize response to return before progress reports | ||
| Process.sleep(50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. Perhaps it would be an interesting addition for gen_lsp to support something like:
{:reply, reply, {:continue, term, state}} |
{:noreply, {:continue, term, state}}Then support a handle_continue callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching GenServer semantics? Yeah, that'd be sweet.
The key to resolving the timing issue here is to make sure the language-client gets the initialize response, before the language-server initializes anymore workDoneTokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be the idea. So that we can support a situation like this where we want to perform work on a continuation after the current work is done, with some guarantee the reply was sent - although there might be other valid reasons to want to do a continuation I guess. E.g.:
def handle_request(req, lsp) do
reply = get_reply(req)
{:reply, reply, {:continue, :initialize_engine, lsp}}
end
def handle_continue(:initialize_engine, lsp) do
# maybe something related but we want to offload that particular loop iteration
# maybe something unrelated and we want to separate scope
# maybe something that should run in some continuous "work" manner
# maybe we wanted some guarantee the reply was sent
{:noreply, lsp}
endAnyway, not an issue for this particular PR, I wondered if you'd tell me if this sounds like a reasonable "solution" for the sleep issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a crack at this: https://github.com/Moosieus/gen_lsp/tree/moo/handle-continue
Not tested in-anger against Expert at time of writing, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look's great!
This pull request overhauls the way progress reporting is handled in Expert. The top-level API remains largely the same for callers.
The prior architecture started a per-project stateful GenServer that handled progress tracking logic internally:
with_client_progressfunction incorporated here, but I removed it for the sake of review. There’s already a lot here.The new progress reporting modules are organized as follows:
Forge.Progress- a behaviour that defines thewith_progressandwith_tracked_progresshelper functions, permitting the implementing module provides the following functions:begin/2report/2complete/2Expert.Progress- a process-less module that directly handles progress reporting, implements the aforementionedEngine.Progress- a thin wrapper that callsExpert.Progressvia erpc.Forge.Progress.Tracker- An ephemeral GenServer for reporting progress between concurrently running tasks.turbo-encabulating files: 3/25should that be preferable.All in all: Less processes, more sequential Elixir, more flexibility, better alignment with LSP semantics.
Some additional changes:
project_compiledmessage less flaky by increasing the timeout from 100ms -> 5s.