Skip to content
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

Automatically restart bsp connection when connection socket is closed #1266

Merged
merged 2 commits into from Jan 10, 2020

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Jan 7, 2020

Previously, build connection could fail silently. Now, user is prompted and offered to automatically restart bsp connection when socket is closed

Haven't added tests, since they would be most likely flaky.

Fixes #1191

reconnect

@tgodzik tgodzik requested review from olafurpg and jvican Jan 7, 2020
…ed and offered to automatically restart bsp connection when socket is closed
@tgodzik tgodzik force-pushed the tgodzik:auto-reconnect branch from 9429150 to fbfc22b Jan 8, 2020
@jvican
jvican approved these changes Jan 8, 2020
Copy link
Collaborator

jvican left a comment

Yay, LGTM! 👍 Had a cursory look at the code and I don't have anything to add.

I'm looking forward to trying this out and check if this fixes the spurious disconnections that happen in my MacBook Pro when I close its lid 😎

@tgodzik tgodzik requested review from ckipp01 and marek1840 Jan 8, 2020
@ckipp01
ckipp01 approved these changes Jan 9, 2020
Copy link
Member

ckipp01 left a comment

As far as I can tell this looks great! LGTM 🚀

Copy link
Member

olafurpg left a comment

I just tried this locally and it's working as expected. The first time it took a while for the "lost connection" prompt to appear but the second time to came up immediately.

One thing we might want to consider is that if the user selects "Not now" then the prompt will come up again next time you save a file. I'm not sure we want to provide a "Don't show again" option but we might want to add some option in the future to say "I don't want to use a build server", which will put Metals into a mode where there is no build tool.

I'm super excited to see this problem fixed since I believe a lot of "Metals doesn't work" issues are caused by broken build connections!


val params = new ShowMessageRequestParams()
params.setMessage(
s"""|The workspace has lost connection with the Build server, which means most functionalities will not be working.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 9, 2020

Member
Suggested change
s"""|The workspace has lost connection with the Build server, which means most functionalities will not be working.
s"""|Metals lost connection with the build server, most functionality will not work. To fix this problem, select "reconnect to build server".

?

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Jan 9, 2020

I just tried this locally and it's working as expected. The first time it took a while for the "lost connection" prompt to appear but the second time to came up immediately.

Yeah, I am not sure why that happens. Despite Bloop being down, the stream doesn't even react when sending information there. Not sure how to solve that really, usually it popped back up if I unfocused VS Code and went back to try compile. Later reacted immediately.

One thing we might want to consider is that if the user selects "Not now" then the prompt will come up again next time you save a file. I'm not sure we want to provide a "Don't show again" option but we might want to add some option in the future to say "I don't want to use a build server", which will put Metals into a mode where there is no build tool.

Ok, I will add that, but with an information that it's highly not recommended.

Copy link
Collaborator

marek1840 left a comment

Overall there is a lot of changes in this PR with the BuildServerConnection and they are introducing a lot of complexity. Could we somehow simplify how this is implemented?
I was thinking along the lines of having something like:

case class BuildServerConnection(
    reconnect: () => Future[LauncherConnection], // this should ask the user whether to reconnect and, no need implement asking inside the BuildClassConnection if it can come as a parameter
    initialConnection: LauncherConnection,
  (...)
) {
  // the var is necessary, as the `update` methods on AtomicReference expect a function without side-effects and reconnecting is inherently side-effectful.
  @volatile private var connection : Future[LauncherConnection] = Future.successful(LauncherConnection)

  private def request(f: LauncherConnection => Future[A]): Future[A] = {
    val original = connection.get()
    f(original).recover {
      case _: IOException =>
          // by 
          // a) comparing the current connection with the one we used to issue the request, 
          // b) synchronizing over this block of code and 
          // c) ensuring the connection changes only here, 
          // we can be sure that the connection changes only one per failed request made on that specific connection
          synchronized {
            if (connection eq original) connection = reconnect()
          }
          request(f) // TODO we should probably limit the recovery attempts
    }
  }

  // now here, we just need to register and don't have to bother with the reconnecting
  private def register[T](
      action: MetalsBuildServer => CompletableFuture[T]
  ): CompletableFuture[T] = {
    val future = request(connection => action(connection.server))
      .{result => 
         OngoingRequests.add(...)
         result.asScala
       }
    CancelTokens.future(_ => future)
  }
}
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Jan 9, 2020

@marek1840 Thanks for the review - helped me to simplify the code greatly!

@tgodzik tgodzik requested a review from marek1840 Jan 9, 2020
@tgodzik tgodzik force-pushed the tgodzik:auto-reconnect branch from b0c6f32 to 0f92bf4 Jan 9, 2020
@marek1840

This comment has been minimized.

Rename to currentVersion

@tgodzik tgodzik merged commit 60d73c7 into scalameta:master Jan 10, 2020
12 checks passed
12 checks passed
windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@tgodzik tgodzik deleted the tgodzik:auto-reconnect branch Jan 10, 2020
@olafurpg

This comment has been minimized.

I don't think we should offer this option. Metals does not function without a build server connection. I can imagine users will select this option and then report issues when "Metals doesn't work" not knowing that they dismissed the notifications.

I think it's more reasonable that Not now dismisses the notification for something like two minutes.

This comment has been minimized.

Copy link
Collaborator Author

tgodzik replied Jan 10, 2020

och, alright - I didn't then understand fully what you meant

This comment has been minimized.

Copy link
Collaborator Author

tgodzik replied Jan 10, 2020

Addressed in #1282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.