Reworked Bloop connection and Tree View #1677

tgodzik · 2020-04-29T17:00:22Z

Previously, we would only reconnect if the user asked for it and often this would not show up quickly.
Now, we make the reconnection work better by:

reconnecting as soon as the build server is disconnected (fixes Missing bloop disconnection alert when exiting bloop #1354)
making reconnection automatic unless the user specifically asks for it not to be via system property metals.ask-to-reconnect=true
moving compilation outside the initial build server connection - longer running compilations would then produce invalid exceptions and break the next connection
reindexing and compiling the workspace on reconnection (this is needed since workspaces would not have all the classpath updated)

Also in this PR:

a new clean-compile command was introduced, which together with the restart bloop command was added to tree view
tree view now has 4 parts - one for help, one for project view, one for commands and one for running compilations

The reworked tree view would look like this:

I am also thinking of:

adding icons to compiling projects, I've seen it break in some cases
partly reindexing the workspace - not sure if that is sensible since Bloop has a different directory for each client and when reconnecting we are effectively a different client

tgodzik · 2020-04-29T17:02:59Z

@nilskp I changed the TreeView to make it more explicit that the commands are clickable and add two more useful commands. Will this be helpful?

tgodzik

Left some notes on what I changed and why. Overall, I think we need the improvements here to help with the issues we are having in Bloop. In the meantime, I am also trying to help with those issues in Bloop.

@ckipp01 Could you maybe take a look at this from the coc-metals perspective? Not sure if a lot of changes will be need or whether I should do something differently.

tgodzik · 2020-04-29T17:17:04Z

metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala

@@ -96,13 +98,15 @@ final class BloopServers(
        serverStarted
      )

+    val finished = Promise[Unit]()


This promise is used to determine whether the launcher is finished.

then maybe it could be called: launched instead of finished?

finished is updated when the launcher finishes and not when it launches, so I think this is better.

tgodzik · 2020-04-29T17:17:54Z

metals/src/main/scala/scala/meta/internal/metals/BspServers.scala

@@ -70,14 +72,21 @@ final class BspServers(
        s"${details.getName} input stream"
      )

+      val finished = Promise[Unit]()
+      Future {
+        process.waitFor()


Not 100% sure about this solution, need to test it with another BSP server.

Added a test for it in Bill Suite, seems to work pretty well.

tgodzik · 2020-04-29T17:18:46Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

 )(implicit ec: ExecutionContextExecutorService)
    extends Cancelable {

  @volatile private var connection = Future.successful(initialConnection)
+  connection.foreach { conn => conn.onConnectionFinished(reconnect) }


This uses the promise from the launcher to reconnect when the connection is finished.

I'm not sure I understand the logic here, what is the difference between this and the logic below?

Suggested change

connection.foreach { conn => conn.onConnectionFinished(reconnect) }

initialConnection.onConnectionFinished(reconnect)

onConnectionFinished is coming from the process or the launcher to say that the connection is finished. onReconnection is used to later reindex the workspace.

The alternative is to do it in the MetalsLanguageServer and just do a full reconnect, but then it will be less seamless for the user.

LauncherConnection stoped -> BuildServerConnection.reconnect -> MetalsLanguageServer.indexWorkspace

I think this is a bit better than a full reconnect, because we will not lose any requests to the build server.

tgodzik · 2020-04-29T17:19:25Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

+  def onReconnection(
+      index: BuildServerConnection => Future[Unit]
+  ): Unit = {
+    onReconnection.set(index)


We need to reindex the workspace and this is to make sure that we will only try to reindex once we finished the indexing at least once.

tgodzik · 2020-04-29T17:19:51Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

-        case response if response == Messages.DisconnectedServer.notNow =>
-          notification.dismiss(5, TimeUnit.MINUTES)
-          connection
+    if (config.askToReconnect) {


Depends now on the user configuration.

tgodzik · 2020-04-29T17:20:18Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

+          conn.onConnectionFinished(reconnect)
+          conn
+        }
+        connection.foreach(_ => onReconnection.get()(this))


Here we actually specify that we want to reindex.

tgodzik · 2020-04-29T17:22:00Z

metals/src/main/scala/scala/meta/internal/metals/Compilations.scala

@@ -66,6 +66,26 @@ final class Compilations(
    cascadeBatch.cancelCurrentRequest()
  }

+  def recompile(): Future[Unit] = {


Used to fully recompile a workspace.

if the fully part is more important, then maybe it should be called compileAll ?

Changed to recompileAll

tgodzik · 2020-04-29T17:23:13Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

-      )
-      scribe.error(message, e)
-      BuildChange.Failed
+      .flatMap(compileAllOpen)


Moved compiling after recover, since this caused to disconnect the build server if anything happened during the compilation.

tgodzik · 2020-04-29T17:23:39Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

+            val result = connectToNewBuildServer(build)
+            build.onReconnection { reconnected =>
+              connectToNewBuildServer(reconnected)
+                .flatMap(compileAllOpen)


We also compile, which will effectively just move all the old artifacts to the new BSP directory.

tgodzik · 2020-04-29T17:26:11Z

metals/src/main/scala/scala/meta/internal/tvp/TreeViewNode.scala

    TreeViewNode(
      viewId = "commands",
      nodeUri = s"metals://command/${command.id}",
      label = command.title,
      command = MetalsCommand(
        command.title,
-        command.id,
+        "metals." + command.id,


Somehow this didn't work with the new command, since the ones declared all have the metals. prefix

This might not be needed, but I haven't been able to make the new command work otherwise.

I think we could also just use an echo command for all server commands here.

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala

metals/src/main/scala/scala/meta/internal/metals/ServerCommands.scala

ckipp01 · 2020-04-29T18:26:39Z

@ckipp01 Could you maybe take a look at this from the coc-metals perspective? Not sure if a lot of changes will be need or whether I should do something differently.

First try shows the MetalsBuild section just fine, but the MetalsCompile and is empty and MetalsProject doesn't show up :/. Not sure if it's just a small thing or a larger thing that needs to get fixed on the coc-metals side without diving further in.

tgodzik · 2020-04-29T18:32:58Z

Seems to be an issue with shutting down currently. The connection might get automatically reestablised 😅 Will fix it tomorrow.

tgodzik · 2020-04-29T18:34:54Z

@ckipp01 Could you maybe take a look at this from the coc-metals perspective? Not sure if a lot of changes will be need or whether I should do something differently.

First try shows the MetalsBuild section just fine, but the MetalsCompile and is empty and MetalsProject doesn't show up :/. Not sure if it's just a small thing or a larger thing that needs to get fixed on the coc-metals side without diving further in.

Project just changed the name, so maybe we can adjust the names of views too? MetalsCompile should be empty unless we are compiling anything.

olafurpg

I'm really excited to see this PR! I believe it will be a big improvements in troubleshooting errors when Metals is behaving weirdly. A few notes on the names, I haven't reviewed the code.

Should "Recompile workspace" be called "Clean compile workspace" instead?

Should we use the sync icon 🔄 for "Import build" for consistency with IntelliJ "Refresh project"? We can use something like a stop icon ⏹️ for "Restart build server" instead.

Can we move "Restart build server" to the bottom of the command list in the tree view?

(brainstorming, maybe for a separate PR) I wonder if we should have some form of "Clean import build" which removes .bloop/ before importing the build 🤔 I worry however that we end up having too many troubleshooting commands and users blindly run them all.

tgodzik · 2020-05-01T10:03:25Z

Should "Recompile workspace" be called "Clean compile workspace" instead?

I was thinking about it, but recompile seemed to be clear that we are redoing the whole compilation. I can change don't feel strongly about it

Should we use the sync icon for "Import build" for consistency with IntelliJ "Refresh project"? We can use something like a stop icon for "Restart build server" instead.

Good idea! I wasn't really sure about those two.

Can we move "Restart build server" to the bottom of the command list in the tree view?

Sure!

(brainstorming, maybe for a separate PR) I wonder if we should have some form of "Clean import build" which removes .bloop/ before importing the build I worry however that we end up having too many troubleshooting commands and users blindly run them all.

Honestly, I have never needed to do that myself. Either clean compile or restart server should help with most situations. There is no need to remove the .json files in .bloop ever, so the only thing we need to do is remove the compile artifacts, which the clean compile should do.

tgodzik · 2020-05-01T11:32:47Z

@olafurpg I updated the icons for import and restart server. I used debug-stop for restart, since stop gives us the same icon as for cancel. I also updated the VSCode PR so that if we don't find an icon we try to use the default ones from VS Code.

The image in the description got updated too.

Btw. you can actually easily look for icons in https://microsoft.github.io/vscode-codicons/dist/codicon.html

And they are quite easy to modify in https://www.figma.com/, so it shouldn't take me too long to update anything if anyone has better ideas.

ckipp01 · 2020-05-03T09:45:51Z

metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala

@@ -75,6 +76,12 @@ class MetalsTreeViewProvider(
    languageClient.metalsTreeViewDidChange(
      TreeViewDidChangeParams(
        Array(
+          TreeViewNode(


Sort of nitpicky, but I'm trying to debug what is going on with coc-metals with the branch, and one things I noticed is that this is technically against the spec that we have published. In the TreeViewNode class, viewId and lable aren't marked as @Nullable, and in the spec it's the same thing. I'm getting an undefined that I'm trying to chase down, and I'm sure it's coming from here, but this is the sort of thing that could cause it and confuse someone since there should be somewhat of a guarantee that there is a label here. The same for the other two TreeViewNode's below this.

Added TreeViewNode.empty that sets both the id and label.

tgodzik · 2020-05-04T09:05:26Z

metals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scala

    TreeViewNode(
      Compile,
-      "metals://ongoing-compilations",


@ckipp01 Maybe this part is a problem with the undefined ? I hoped the node is discovered by id, but maybe we check something else in vim? This is the only big change, the rest is just moving things around

No, I found the issue. The TVP panel in coc-metals was sort of built with the assumption that apart from the initial view it was never empty. Now that things have been shifted around the the compilations view will return an empty array just isn't accounted for. So I need to adjust the logic for this since I have logic built-in to find the offset of node to update, which is wrong now.

Ok, cool. Thanks for taking a look at it!

mzarnowski · 2020-05-04T09:03:37Z

metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala

@@ -96,13 +98,15 @@ final class BloopServers(
        serverStarted
      )

+    val finished = Promise[Unit]()


then maybe it could be called: launched instead of finished?

mzarnowski · 2020-05-04T09:07:35Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

  /** Run build/shutdown procedure */
  def shutdown(): Future[Unit] = connection.map { conn =>
    try {
+      isShuttingDown.set(true)


by using compareAndSet(false, true you could guarantee here that the method will be called exactly once, hence making it idempotent.

Good idea! Fixed it!

mzarnowski · 2020-05-04T09:10:51Z

metals/src/main/scala/scala/meta/internal/metals/Compilations.scala

@@ -66,6 +66,26 @@ final class Compilations(
    cascadeBatch.cancelCurrentRequest()
  }

+  def recompile(): Future[Unit] = {


if the fully part is more important, then maybe it should be called compileAll ?

mzarnowski · 2020-05-04T09:12:41Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

-      }
-      _ = {
-        treeView.init()
+    def compileAllOpen: BuildChange => Future[BuildChange] = {


could be compileOpenFiles at first I was not 100% if it meant open files.

changed to compileAllOpenFiles

…estart-build commands to it. Also, make reconnection work better by: - reconnecting as soon as the build server is disconnected - making reconnection automatic unless the user specifically asks for it not to be - moving compilation outside the initial build server connection - longer running compilations would then produce invalid exceptions

tgodzik mentioned this pull request Apr 29, 2020

Add new icons for the metals view and a separate compilations section scalameta/metals-vscode#289

Merged

tgodzik force-pushed the rearrange-tree-view branch from 345ae6a to 1d7362d Compare April 29, 2020 17:16

tgodzik commented Apr 29, 2020

View reviewed changes

tgodzik force-pushed the rearrange-tree-view branch 2 times, most recently from 2c870e8 to aabc8e3 Compare April 29, 2020 18:17

ckipp01 reviewed Apr 29, 2020

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/MetalsServerConfig.scala Show resolved Hide resolved

metals/src/main/scala/scala/meta/internal/metals/ServerCommands.scala Outdated Show resolved Hide resolved

tgodzik force-pushed the rearrange-tree-view branch 3 times, most recently from caa9aac to e2c4358 Compare April 30, 2020 08:41

olafurpg reviewed May 1, 2020

View reviewed changes

tgodzik force-pushed the rearrange-tree-view branch from e2c4358 to 86dce71 Compare May 1, 2020 11:26

tgodzik force-pushed the rearrange-tree-view branch from 86dce71 to db17161 Compare May 1, 2020 15:53

ckipp01 reviewed May 3, 2020

View reviewed changes

ckipp01 mentioned this pull request May 3, 2020

Initial settings needed for new treeview changes scalameta/coc-metals#190

Merged

ckipp01 added the improvement Not a bug or a feature, but something general we can improve label May 4, 2020

tgodzik commented May 4, 2020

View reviewed changes

mzarnowski approved these changes May 4, 2020

View reviewed changes

tgodzik force-pushed the rearrange-tree-view branch 3 times, most recently from e1444fe to 079c8f7 Compare May 4, 2020 11:18

ckipp01 mentioned this pull request May 4, 2020

Account for upstream TVP changes for compilations view to work scalameta/coc-metals#192

Closed

tgodzik force-pushed the rearrange-tree-view branch from 079c8f7 to 3875129 Compare May 4, 2020 13:57

tgodzik merged commit a39ac72 into scalameta:master May 4, 2020

tgodzik deleted the rearrange-tree-view branch May 4, 2020 14:55

tgodzik mentioned this pull request May 7, 2020

"Failed to connect with build server, no functionality will work. See logs for more details." #1627

Closed

tgodzik mentioned this pull request Aug 6, 2020

Clean build scalameta/metals-feature-requests#26

Closed

	connection.foreach { conn => conn.onConnectionFinished(reconnect) }
	initialConnection.onConnectionFinished(reconnect)

Reworked Bloop connection and Tree View #1677

Reworked Bloop connection and Tree View #1677

Conversation

tgodzik commented Apr 29, 2020 • edited Loading

tgodzik commented Apr 29, 2020

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 commented Apr 29, 2020

tgodzik commented Apr 29, 2020

tgodzik commented Apr 29, 2020

olafurpg left a comment

Choose a reason for hiding this comment

tgodzik commented May 1, 2020

tgodzik commented May 1, 2020 • edited Loading

ckipp01 May 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 May 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Apr 29, 2020 •

edited

Loading

tgodzik May 1, 2020 •

edited

Loading

tgodzik commented May 1, 2020 •

edited

Loading

ckipp01 May 3, 2020 •

edited

Loading

ckipp01 May 4, 2020 •

edited

Loading