[jb] enable connect button only when JetBrains Client not activated #10177

yaohui-wyh · 2022-05-22T13:46:08Z

Description

Each time user clicked on the "connect" button, previous activated JetBrainsClient is restarted. Since JBClient takes quite a long startup time, disable the connect button to prevent unexpected restart might be reasonable.

This PR syncs the jbclient's state for workspace view: when a JetBrainsClient is launched for a Gitpod workspace (via JetBrains Gateway Plugin), update the "connect" button text as "connected", and disable the connect button until the JetBrainsClient is closed

Related Issue(s)

How to test

Build & Install the plugin following
https://github.com/gitpod-io/gitpod/blob/main/components/ide/jetbrains/gateway-plugin/README.md#L8

Release Notes

JetBrains Gateway: The "Connect" button now gets disabled while a JetBrains Client is connected to the workspace.

/werft no-preview=true

felladrin · 2022-05-23T08:07:35Z

/werft run

👍 started the job as gitpod-build-jb-gw-fork.0
(with .werft/ from main)

Nice addition!
I'm gonna review it soon.

felladrin · 2022-05-23T11:32:04Z

It's working great!

One thing I've noticed though, is that if we close the Client, it will only change back the button to "Connect" (instead of "Connected") if we click the Refresh button. I think it might be a problem. Ideally, we should also have a listener (or a long-polling) to reset the button when the Thin Client gets closed.

yaohui-wyh · 2022-05-23T13:00:56Z

One thing I've noticed though, is that if we close the Client, it will only change back the button to "Connect" (instead of "Connected") if we click the Refresh button. I think it might be a problem. Ideally, we should also have a listener (or a long-polling) to reset the button when the Thin Client gets closed.

Hey @felladrin! That's a good catch!!! Thanks for your thorough testing!
And your suggested fix is absolutely correct, I did miss the dispatcher.multicaster.didChange() in the client.clientClosed event listener. I have pushed the fix at 4618773, and I will rebase the commits and force push again.

Previously I thought the JBClient closing event would fire at the client.onClientPresenceChanged, and now it turns out it won't. During my test, the "Connected" button sometimes changed backed to "Connect" when local JBClient exit on connection broken. Now I think it was because the client switch to some failure state (but not closing state). And to handle the closing state we need to subscribe to the clientClosed

Gateway SDK is a black box sadly, I'm quite curious how @akosyakov gets to know the extension points of Gateway & figures out which apis to use :-) If we have more docs about the Gateway SDK, we could avoid lots of unexpected behaviors

akosyakov · 2022-05-23T13:08:48Z

Gateway SDK is a black box sadly, I'm quite curious how @akosyakov gets to know the extension points of Gateway & figures out which apis to use :-) If we have more docs about the Gateway SDK, we could avoid lots of unexpected behaviors

Logging and trying 😆

felladrin · 2022-05-23T17:41:54Z

I have pushed the fix at 4618773, and I will rebase the commits and force push again.

Thanks for fixing it @yaohui-wyh!
I see it refreshing the list after I close the IDE, but a new problem came up: it updates the list a few milliseconds before GitpodConnectionProvider.jetbrainsClientMap[info.workspace.id]?.clientPresent becomes false. So I see the list updating (I see it blinking and repositioning), but the button is still "Connected" after I close the Thin Client. And if, right after the update, I click (the fastest I can) the "Refresh" button, it updates to "Connect". Are you able to reproduce it?

Maybe we could delay dispatcher.multicaster.didChange() in 1 second here:

gitpod/components/ide/jetbrains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt

Lines 258 to 260 in 24efeeb

    
           client.clientClosed.advise(connectionLifetime) { 
        
               application.invokeLater { 
        
                   dispatcher.multicaster.didChange()

There might be a better solution than using a timer, but I couldn't think on anything else at this moment (intellisense inside gateway-plugin folder stopped working for me).

yaohui-wyh · 2022-05-24T03:59:09Z

Hi @felladrin, Thanks again for the thorough testing!

... the button is still "Connected" after I close the Thin Client.

Yeah, I can reproduce it. (And sorry for that)
Actually I found out the "Connected" state won't switch back to "Connect" after the ThinClient closed most of the time. It updates to "Connect" only when I click the "Refresh" button.

So the previous attempt (add dispatcher.multicaster.didChange() in the client.clientClosed did passed the event, however the state between ConnectionProvider and WorkspaceView is not synced)

And after digging into the properties of ThinClient:

Which callback listener to subscribe: after logging and trying @akosyakov, I think we should move dispatcher.multicaster.didChange() to connectionLifetime.onTermination callback (rather than client.clientClosed)
Which state to check for ThinClient's liveness: ThinClient's lifeTime.isAlive is the reliable way to check if JBClient is terminated, rather than clientPresent

2022-05-24 11:31:53,515 [jb-gw] clientClosed, uid=7d88d4fa-1c88-44f2-91a1-148d9009f43c, clientPresent=true, lifeTime={status=Alive, alive=true}
2022-05-24 11:31:53,528 [jb-gw] clientTermination, clientClosed didChange fired, uid=7d88d4fa-1c88-44f2-91a1-148d9009f43c, clientPresent=true, lifeTime={status=Canceled, alive=false}
2022-05-24 11:31:53,530 [jb-gw] WorkspaceView refresh, [wsId=gitpodio-springpetclini-n6bq52vs609, uid=7d88d4fa-1c88-44f2-91a1-148d9009f43c, clientPresent=true, lifeTime={status=Canceled, alive=false}]

Note that the clientPresent was always true, while lifeTime reflected the correct state of JBClient

In short, we should rely on Lifetime of RD protocol (which is still a myth by now), but not ThinClient's internal state. ThinClient does hold a lifeTime property and we should use that.

yaohui-wyh · 2022-05-24T04:03:21Z

The exact fix is only ~~one~~two line

@@ -256,13 +259,19 @@ class GitpodConnectionProvider : GatewayConnectionProvider {
                                     )
                                     val client = connector.connect()
                                     client.clientClosed.advise(connectionLifetime) {
                                         application.invokeLater {
-                                            dispatcher.multicaster.didChange()
+                                            connectionLifetime.onTermination {
+                                                dispatcher.multicaster.didChange()
+                                            }
                                             connectionLifetime.terminate()
                                         }
                                     }

@@ -295,7 +301,7 @@ class GitpodWorkspacesView(
                                 }
                                 label(getRelativeTimeSpan(info.latestInstance.creationTime))
                                 val jbClientConnected = GitpodConnectionProvider.jetbrainsClientMap[info.workspace.id]
-                                    ?.clientPresent ?: false
+                                    ?.lifetime?.isAlive ?: false
                                 val btnText = if (jbClientConnected) "Connected" else "Connect"
                                 button(btnText) {
                                     if (!canConnect) {

However I think it would be better to keep the logging statements & extension functions, since they are not on the critical path and wouldn't jam the log.

...rains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt

felladrin

It's working great now! Thanks for this PR @yaohui-wyh!

felladrin · 2022-05-24T09:45:45Z

...rains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt

                                    client.clientClosed.advise(connectionLifetime) {
+                                        thisLogger().d("clientClosed, ${client.prettyPrint()}")
                                        application.invokeLater {
+                                            connectionLifetime.onTermination {
+                                                thisLogger().d("clientTermination, clientClosed didChange fired, ${client.prettyPrint()}")
+                                                dispatcher.multicaster.didChange()
+                                            }
                                            connectionLifetime.terminate()
                                        }
                                    }


One thing I've noticed, is that we could benefit from letting the connectionLifetime.onTermination set even before the client gets closed. Because there is another place where we call connectionLifetime.terminate(), and in the future we can have even more cases.
Something like the following, which can be done in a follow-up PR if needed:

Suggested change

client.clientClosed.advise(connectionLifetime) {

thisLogger().d("clientClosed, ${client.prettyPrint()}")

application.invokeLater {

connectionLifetime.onTermination {

thisLogger().d("clientTermination, clientClosed didChange fired, ${client.prettyPrint()}")

dispatcher.multicaster.didChange()

}

connectionLifetime.terminate()

}

}

connectionLifetime.onTermination {

thisLogger().d("clientLifetimeTermination, didChange fired, ${client.prettyPrint()}")

dispatcher.multicaster.didChange()

}

client.clientClosed.advise(connectionLifetime) {

thisLogger().d("clientClosed, ${client.prettyPrint()}")

application.invokeLater {

connectionLifetime.terminate()

}

}

akosyakov · 2022-05-24T09:52:45Z

...rains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt

@@ -52,6 +57,26 @@ class GitpodConnectionProvider : GatewayConnectionProvider {

    private val gitpod = service<GitpodConnectionService>()

+    companion object {
+        var jetbrainsClientMap: MutableMap<String, ThinClientHandle> = mutableMapOf()


When do we remove a client from this map?

(nit) Should not it also be private and we expose instead getClient?

akosyakov · 2022-05-24T10:09:20Z

This PR is leaking thin clients and does not look very structured :(

akosyakov · 2022-05-24T10:17:05Z

...jetbrains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodWorkspacesView.kt

@@ -293,7 +300,10 @@ class GitpodWorkspacesView(
                                    )
                                }
                                label(getRelativeTimeSpan(info.latestInstance.creationTime))
-                                button("Connect") {
+                                val jbClientConnected = GitpodConnectionProvider.jetbrainsClientMap[info.workspace.id]


Should not we rather focus existing client instead?

This is a good idea, however I haven't found a way to requestFocus for the ThinClient yet

akosyakov · 2022-05-24T10:20:12Z

...rains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt

                                            if (client.clientPresent) {
                                                statusMessage.text = ""
                                            }
                                        }
                                    }
                                    thinClient = client
+                                    jetbrainsClientMap[update.workspaceId] = client


Should not it be like:

jetbrainsClientMap[update.workspaceId] = client dispatcher.multicaster.didChange() connectionLifetime.onTermination { jetbrainsClientMap.remove(update.workspaceId) dispatcher.multicaster.didChange() }

We could also create a private function setCleint(id, client | null) and then do here:

setClient(id, client); connectionLifetime.onTermination { setClient(id, null) }

akosyakov · 2022-05-24T10:23:41Z

@yaohui-wyh Will you send a clean up PR? of @felladrin takes care about clean up?

yaohui-wyh · 2022-05-24T10:27:13Z

@yaohui-wyh Will you send a clean up PR? of @felladrin takes care about clean up?

Sure, I will make a clean up PR. Thanks for the comments:) @akosyakov @felladrin

yaohui-wyh requested a review from a team May 22, 2022 13:46

roboquat added release-note size/M labels May 22, 2022

iQQBot added community-contribution cla: accepted ✔️ labels May 22, 2022

akosyakov added the team: IDE label May 22, 2022

yaohui-wyh force-pushed the jb-gw branch from 4618773 to 24efeeb Compare May 23, 2022 13:02

[jb] enable connect button only when JetBrains Client not activated

cdfb2bb

yaohui-wyh force-pushed the jb-gw branch from e0ce000 to cdfb2bb Compare May 24, 2022 04:06

yaohui-wyh commented May 24, 2022

View reviewed changes

...rains/gateway-plugin/src/main/kotlin/io/gitpod/jetbrains/gateway/GitpodConnectionProvider.kt Show resolved Hide resolved

felladrin approved these changes May 24, 2022

View reviewed changes

roboquat merged commit bd8f2c7 into gitpod-io:main May 24, 2022

akosyakov reviewed May 24, 2022

View reviewed changes

yaohui-wyh mentioned this pull request May 24, 2022

[jb] enable connect button only when JetBrains Client not activated (cont.) #10216

Closed

1 task

roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels May 25, 2022

felladrin mentioned this pull request May 26, 2022

Revert "[jb] enable connect button only when JetBrains Client not activated" #10279

Merged

loujaybee requested a review from a team May 30, 2022 11:47

[jb] enable connect button only when JetBrains Client not activated #10177

[jb] enable connect button only when JetBrains Client not activated #10177

Uh oh!

Conversation

yaohui-wyh commented May 22, 2022 • edited by felladrin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Uh oh!

felladrin commented May 23, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felladrin commented May 23, 2022

Uh oh!

yaohui-wyh commented May 23, 2022

Uh oh!

akosyakov commented May 23, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felladrin commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaohui-wyh commented May 24, 2022

Uh oh!

yaohui-wyh commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

felladrin left a comment

Choose a reason for hiding this comment

Uh oh!

felladrin May 24, 2022

Choose a reason for hiding this comment

Uh oh!

akosyakov May 24, 2022

Choose a reason for hiding this comment

Uh oh!

akosyakov May 24, 2022

Choose a reason for hiding this comment

Uh oh!

akosyakov commented May 24, 2022

Uh oh!

akosyakov May 24, 2022

Choose a reason for hiding this comment

Uh oh!

yaohui-wyh May 24, 2022

Choose a reason for hiding this comment

Uh oh!

akosyakov May 24, 2022

Choose a reason for hiding this comment

Uh oh!

akosyakov commented May 24, 2022

Uh oh!

yaohui-wyh commented May 24, 2022

Uh oh!

Uh oh!

yaohui-wyh commented May 22, 2022 •

edited by felladrin

Loading

felladrin commented May 23, 2022 •

edited by werft-gitpod-dev-com bot

Loading

akosyakov commented May 23, 2022 •

edited by werft-gitpod-dev-com bot

Loading

felladrin commented May 23, 2022 •

edited

Loading

yaohui-wyh commented May 24, 2022 •

edited

Loading