Fix formatter race condition

[tw4452852]

Formatting request is usually sent immediately after didSave notification, this could cause formatter doesn't work while building is running.

On my end, this is the error log:

LSP Warning: warning: Unable to get formatter options for /home/tw/code/elixir-test/chatter/mix.exs: Mix.Error Unknown dependency :ecto given to :import_deps in the formatter configuration. The dependency is not listed in your mix.exs for environment :test
  (language_server 0.9.0) lib/language_server/source_file.ex:349: ElixirLS.LanguageServer.SourceFile.formatter_opts/1
  (language_server 0.9.0) lib/language_server/providers/formatting.ex:8: ElixirLS.LanguageServer.Providers.Formatting.format/3
  (language_server 0.9.0) lib/language_server/server.ex:783: anonymous fn/3 in ElixirLS.LanguageServer.Server.handle_request_async/2

In order to fix this race condition, let's wait building done before formatting.

Fix #402

Signed-off-by: Tw [email protected]

[lukaszsamson]

Will it not block formatter when build is in progress?

[tw4452852]

Hi @lukaszsamson

Yes, actually it's feasible. The culprit of formatter misfunction is the empty project stack due to this before building:

https://github.com/elixir-lsp/elixir-ls/blob/7f37d59ffe7952d70ddc2f44100227d558c8ef6e/apps/language_server/lib/language_server/build.ex#L130

I don't quite understand the code above, could you give me some insights? Thanks.

[lukaszsamson]

I don't quite understand the code above, could you give me some insights? Thanks.

I don't really know that part either. @axelson, can you help here? Do you think it's a good idea to block formatting on build? I'm afraid it will lead to poor UX.

[axelson]

I don't really know that part either. @axelson, can you help here? Do you think it's a good idea to block formatting on build? I'm afraid it will lead to poor UX.

I also would not like the formatting to rely on a built project. Because for some people it may take 3+ minutes to build their project and that would be a very long time to be waiting for formatting to complete (especially because many people want to format on save).

My preferred fix for this is to make getting the formatter configuration more reliable by changing Elixir to allow us to get the formatter configuration without relying on the current working directory (since that changes as code is compiling). José has stated that a PR would be accepted to fix this, and I have started on one but it is not yet complete (I haven't had much time for open source development recently). After we get the PR merged to Elixir then we can backport the changes into ElixirLS (switching out the implementation based on the Elixir version).

[tw4452852]

Hi @lukaszsamson and @axelson

Indeed, letting formatting wait for the entire building done is unacceptable, so today I worked out another approach.

The culprit for the original issue is the race condition between formatting and project reloading. So if we serialize these two operations, then formatting will only wait project reloading done which is just compiling project's mix.exs under the hood. This will decrease the waiting time dramatically and there's no need to acquire the building lock while formatting.

I tested in my side, it works like a charm. What's your opinion?

[halfdan]

This behaviour just bit me pretty hard. Is this PR waiting on something or can the maintainer get this merged? I'd be happy to lend some help to move this across the finish line.

[lukaszsamson]

can the maintainer get this merged?

I still need to review and test this. It introduces quite a big changes and I'm not certain it will fix the issue completely in case mix is currently compiling a dependancy or an application under an umbrella. Also the PR has merge conflicts...

[VictorGaiva]

This and #772 make the VSCode extension mostly unusable.

[lukaszsamson]

An alternative solution has been merged in https://github.com/elixir-lsp/elixir-ls/pull/890