Single Provider Process #1264
Single Provider Process #1264
Conversation
Erlang LS historically used one process per provider. This was problematic, since requests handled by different providers could lead to race conditions, causing a server crash. For example, a completion request could not see the latest version of the text in case of incremental text synchronization. There was very little gain in parallelizing providers, so we decided to simplify the architecture and have a single process, instead. There's still room for refactoring and cleanup, but those will be handled as a follow up. As part of the architectural change, we also dropped support for BSP since it has been stalling for too long time. Instead, we will try to implement a simpler solution which allows the extraction of paths from the build system (e.g. via the rebar.conifg). * Drop BSP Support * Introduce text synchronization provider * Run providers as a single process * Remove buffer server, index in the background instead * Decouple DAP provider from LSP one * Brutally kill background jobs * Set standard_io as default value for the io_device
There was a problem hiding this comment.
Overall this looks fairly reasonable, just some minor comments.
On the bigger design decisions:
- dropping BSP seems to me like a sensible choice - we don't really have plans to work on that, and could instead integrate with rebar3 more directly
- making the processing less "async" seems like a good choice - main event loop + async processes for specific things seems like a simpler programming model, than every type of request running in parallel
| = State, | ||
| case Provider:handle_request(Request, State) of | ||
| {async, Uri, Job} -> | ||
| {reply, {async, Job}, State#{in_progress => [{Uri, Job}|InProgress]}}; |
There was a problem hiding this comment.
Is there some monitoring of the jobs to remove them from the map when they are externally terminated/crash?
There was a problem hiding this comment.
Not that I can see, even if the consequence of leaving an orphan entry in the map shouldn't be critical. I will create a ticket about this. Also, I now realize that there's some logic running in the terminate function (to clean up the spinning wheel), so I will roll back the brutal_kill strategy bit.
| {noreply, State} | ||
| end. | ||
| ?LOG_DEBUG("Cancelling request [job=~p]", [Job]), | ||
| els_background_job:stop(Job), |
There was a problem hiding this comment.
What happens if we get a cancel request twice for the same job?
|
|
||
| %%============================================================================== | ||
| %% API | ||
| %%============================================================================== | ||
| -spec notify([els_diagnostics:diagnostic()], pid()) -> ok. | ||
| notify(Diagnostics, Job) -> | ||
| ?SERVER ! {diagnostics, Diagnostics, Job}, | ||
| els_provider ! {diagnostics, Diagnostics, Job}, |
There was a problem hiding this comment.
I wonder if those notifications should be somehow incorporated into the background job framework - e.g. that it sends as a reply to the process that started the job the value that was returned from the function (kind of similar to Elixir's Task)
There was a problem hiding this comment.
I'm not familiar with Elixir's Task but it sounds like a good idea. Created #1272
Erlang LS historically used one process per provider. This was
problematic, since requests handled by different providers could lead
to race conditions, causing a server crash. For example, a completion
request could not see the latest version of the text in case of
incremental text synchronization.
There was very little gain in parallelizing providers, so we decided to
simplify the architecture and have a single process, instead. There's
still room for refactoring and cleanup, but those will be handled as a
follow up.
As part of the architectural change, we also dropped support for BSP
since it has been stalling for too long time. Instead, we will try to
implement a simpler solution which allows the extraction of paths from
the build system (e.g. via the rebar.conifg).