Fix listeners leaks – EXP-206 #18321

AlexTugarev · 2023-07-20T13:53:00Z

Description

Before this change, # of listeners plateaued at some level after closing all browser tabs.

After this change, you can see listeners going down to zero.

Changes summary and walkthrough generated by Copilot

`🤖 Generated by Copilot at 2cd5ec4`

This pull request improves the efficiency and accuracy of the server's communication with browser clients and the monitoring of redis metrics. It changes the GitpodServerImpl class to only send updates to relevant clients and fixes a prebuild bug. It also refactors the MonitoringEndpointsApp class to append redis metrics to the default registry.
-->

`🤖 Generated by Copilot at 2cd5ec4`

Refactor monitoring endpoints to append redis metrics to default registry instead of merging them (link)
Add shouldNotifyClients property to GitpodServerImpl class to control whether to send updates to clients about workspace instances and prebuilds (link)
Set shouldNotifyClients property based on client metadata type in initializeClient method of GitpodServerImpl class (link)
Add conditions to only call listenForWorkspaceInstanceUpdates and listenForPrebuildUpdates methods if shouldNotifyClients property is true in initializeClient method of GitpodServerImpl class (link)
Simplify listenForPrebuildUpdates method of GitpodServerImpl class by using push method instead of pushAll method to add disposables (link)
Fix bug in onWorkspaceImageBuildLogs method of GitpodServerImpl class by adding relatedPrebuildFound variable and condition to only create and register resetListenerFromRedis if client exists and prebuild is related to workspace (link)
Add condition to check shouldNotifyClients property before creating and registering prebuildUpdateHandler in createProject method of GitpodServerImpl class (link)

Related Issue(s)

Fixes EXP-206

How to test

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - at-listeners-leak
🔗 URL - at-listeners-leak.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - at-listeners-leak-gha.13861

Build Options

Build

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer

analytics=segment
with-dedicated-emulation
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh
with-monitoring

/hold

components/server/src/workspace/gitpod-server-impl.ts

socket-security · 2023-07-28T10:40:41Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
prom-client	14.2.0	None	`+0`	110 kB	simenb

geropl · 2023-07-28T11:59:51Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -309,12 +311,20 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
        log.debug({ userId: this.userID }, `clientRegion: ${clientHeaderFields.clientRegion}`);
        log.debug({ userId: this.userID }, "initializeClient");

-        this.listenForWorkspaceInstanceUpdates();
+        if (this.client && clientMetadata.type === "browser") {


Why are notifications only relevant for browser clients?
Would be great to have the answer captured in a comment! 🧡

Why are notifications only relevant for browser clients?

Going to add this comment on the final pass.

But in general, you'd disagree about this change? Those events are only consumed by browser clients, no?
cc. @akosyakov on this question.

I'm not sure what the motivation is. Looking at the other client types (source), I think some others are interested in workspace instance updates as well: supervisor, go-client.
And that makes me wonder if it's worth to introduce this distinction here, based on a field that so far we only used for observability purposes. 🤷

In the end, the biggest problem to solve here is the piling up of listener registrations for go-clients, which is AFAIC primarily the PAPI servers holding client connections 🙈

Comment added.

In the end, the biggest problem to solve here is the piling up of listener registrations for go-clients, which is AFAIC primarily the PAPI servers holding client connections

Ok, then I feel I'm lagging context on why we try to limit it here. See Anton's comment.
Maybe we should sync on Monday quickly to get this out the door? 🤔

I see. Didn't check too deeply with all IDE's, but only found no-op handlers for instance updates.
Going to change the PR and only disable prebuild updates to be disabled for non-browser clients.

@akosyakov, I reverted the changes to instance updates.

That said, I believe it would make sense to change the way instance updates are forwarded via PAPI server. If that would directly subscribe on redis, we would be in a position where no connection pooling is required anymore in PAPI server.

Why don't we observe it for instance updates but only for prebuilds?

We reverted the check for browser and proceed with checking for existing clients.

akosyakov · 2023-07-31T07:09:37Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -331,7 +344,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
            );

        for (const projectId of projects) {
-            this.disposables.pushAll([this.subscriber.listenForPrebuildUpdates(projectId, handler)]);
+            this.disposables.push(this.subscriber.listenForPrebuildUpdates(projectId, handler));


here disposables can be already disposed because of async call above? Disposables should be added in sync code or we should checked whether already disposed and then dispose immediately.

Nice catch! I think a DisposableCollection is supposed to be prefilled with single item first, so that if it's emptied once, that state should be kept consistent.

Thanks @svenefftinge and @akosyakov for checking on this.

The PR is updated now, with a different set of changes:

check for client be defined in gitpod-server-impl

add prebuild subscribers only if the thing is not disposed yet

this might have happened frequently on very short living workspacePageClose events

akosyakov

I did not test, but code changes make sense to me.

AlexTugarev · 2023-08-01T15:46:19Z

Still trying to validate on the preview env. Facing unrelated issues with monitoring, though.

- check for client be defined in gitpod-server-impl - add prebuild subscribers only if the thing is not disposed yet - this might have happened frequently on very short living workspacePageClose events

don't override default registry, which breaks other metrics.

AlexTugarev · 2023-08-02T10:27:42Z

Finally, I could validate that prebuild subscribers are going down after closing the tab. I opened the Projects view of the dashboard which rendered 4 items, just to clarify the number visualized here.

Again, the carrier wave here results from the cached PAPI server connections.

AlexTugarev · 2023-08-02T10:27:47Z

/unhold

roboquat added do-not-merge/hold do-not-merge/work-in-progress size/XS labels Jul 20, 2023

AlexTugarev force-pushed the at/listeners-leak branch 2 times, most recently from c50e774 to 2cd5ec4 Compare July 28, 2023 10:05

roboquat added size/M and removed size/XS labels Jul 28, 2023

AlexTugarev changed the title ~~debug logs~~ Fix listeners leaks Jul 28, 2023

AlexTugarev changed the title ~~Fix listeners leaks~~ Fix listeners leaks – EXP-206 Jul 28, 2023

AlexTugarev commented Jul 28, 2023

View reviewed changes

components/server/src/workspace/gitpod-server-impl.ts Outdated Show resolved Hide resolved

AlexTugarev marked this pull request as ready for review July 28, 2023 11:50

AlexTugarev requested a review from a team as a code owner July 28, 2023 11:50

roboquat removed the do-not-merge/work-in-progress label Jul 28, 2023

github-actions bot added the team: team-experience label Jul 28, 2023

geropl reviewed Jul 28, 2023

View reviewed changes

AlexTugarev marked this pull request as draft July 28, 2023 18:18

roboquat added the do-not-merge/work-in-progress label Jul 28, 2023

AlexTugarev force-pushed the at/listeners-leak branch from 376dd42 to 458940c Compare July 28, 2023 18:40

akosyakov reviewed Jul 31, 2023

View reviewed changes

AlexTugarev marked this pull request as ready for review August 1, 2023 08:07

roboquat removed the do-not-merge/work-in-progress label Aug 1, 2023

AlexTugarev force-pushed the at/listeners-leak branch 2 times, most recently from 42c1fcc to 209340a Compare August 1, 2023 08:18

akosyakov approved these changes Aug 1, 2023

View reviewed changes

AlexTugarev added 4 commits August 2, 2023 08:29

[server] fix leaking prebuild update listeners

1f512e9

- check for client be defined in gitpod-server-impl - add prebuild subscribers only if the thing is not disposed yet - this might have happened frequently on very short living workspacePageClose events

[server] fix metric registration

cd2cab1

don't override default registry, which breaks other metrics.

fixup for getMetricsAsArray

917a293

[server] bump prom-client

90495f4

AlexTugarev force-pushed the at/listeners-leak branch from 209340a to 90495f4 Compare August 2, 2023 08:30

roboquat removed the do-not-merge/hold label Aug 2, 2023

roboquat merged commit 436130b into main Aug 2, 2023

roboquat deleted the at/listeners-leak branch August 2, 2023 10:28

Fix listeners leaks – EXP-206 #18321

Fix listeners leaks – EXP-206 #18321

Uh oh!

Conversation

AlexTugarev commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

🤖 Generated by Copilot at 2cd5ec4

🤖 Generated by Copilot at 2cd5ec4

Related Issue(s)

How to test

Documentation

Preview status

Build Options

Uh oh!

Uh oh!

socket-security bot commented Jul 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geropl Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosyakov left a comment

Choose a reason for hiding this comment

Uh oh!

AlexTugarev commented Aug 1, 2023

Uh oh!

AlexTugarev commented Aug 2, 2023

Uh oh!

AlexTugarev commented Aug 2, 2023

Uh oh!

Uh oh!

AlexTugarev commented Jul 20, 2023 •

edited

Loading

`🤖 Generated by Copilot at 2cd5ec4`

`🤖 Generated by Copilot at 2cd5ec4`

geropl Jul 28, 2023 •

edited

Loading

akosyakov Jul 31, 2023 •

edited

Loading