Skip to content

Commit 1409e36

Browse files
authored
fix: DH-22840: DHC connection race (#322)
DH-22840: Fixed a race condition where clicking a community server node multiple times could result in connection count > 1 ### Testing - Configure a community server - While disconnected from the server, click the server node multiple times quickly. Should only see (1) connection and `Output → Deephaven Debug` panel should show a message like `[ServerManager] INFO: Already connected to server: http://localhost:10000/`
1 parent 6acef57 commit 1409e36

8 files changed

Lines changed: 317 additions & 27 deletions

File tree

CONTRIBUTING.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ See [vscode-extension-tester](https://github.com/redhat-developer/vscode-extensi
3535
To run end-to-end tests:
3636

3737
```sh
38-
npm run test:e2e
38+
npm run test:e2e -- --core
3939
```
4040

4141
To run using `VS Code` debugger:
@@ -61,6 +61,7 @@ For release testing:
6161
1. Checkout the appropriate pre-release tag for the pending release.
6262
1. Run `npm install`
6363
1. It is recommended to run against a few different server configurations:
64+
6465
- Server configured with both basic and SAML configs
6566
- Server with basic only config
6667
- Envoy server

scripts/e2e.sh

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,15 @@ fi
8484

8585
export DH_SERVER_URL="${SERVER_URL}"
8686

87+
# Locally, default test resources to a short path outside the repo. VS Code/Electron
88+
# creates an IPC socket under <TEST_RESOURCES>/settings, and macOS caps Unix socket
89+
# paths at 104 bytes; a long repo/worktree path overflows it and the VS Code instance
90+
# exits immediately ("Chrome instance exited"). Skipped in CI (where $CI is set) so
91+
# screenshots keep landing in e2e-testing/.resources for the artifact upload.
92+
if [[ -z "$CI" ]]; then
93+
export TEST_RESOURCES="${TEST_RESOURCES:-$HOME/.dh-e2e-resources}"
94+
fi
95+
8796
# Run e2e tests
8897
echo "Running E2E tests..."
8998
node e2e-testing/out/runner.mjs --setup

scripts/generate-test-settings.mjs

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,16 +92,21 @@ if (coreServers.length === 0 && enterpriseServers.length === 0) {
9292
}
9393

9494
// Create settings object
95+
/* eslint-disable @typescript-eslint/naming-convention */
9596
const settings = {
96-
// eslint-disable-next-line @typescript-eslint/naming-convention
9797
'deephaven.coreServers': coreServers,
98-
// eslint-disable-next-line @typescript-eslint/naming-convention
9998
'deephaven.enterpriseServers': enterpriseServers,
100-
// eslint-disable-next-line @typescript-eslint/naming-convention
10199
'extensions.ignoreRecommendations': true,
102-
// eslint-disable-next-line @typescript-eslint/naming-convention
103100
'git.openRepositoryInParentFolders': 'never',
101+
// Suppress the onboarding overlay (.onboarding-a-overlay, "Welcome to Visual
102+
// Studio Code") that newer VS Code shows on first run. ExTester wipes the
103+
// user-data-dir each run, so every run looks like a first run and the overlay
104+
// reappears, intercepting clicks and blocking the quick-input widget.
105+
// VS Code's tryShowOnboarding() only calls show() when this setting is truthy
106+
// (see workbench.desktop.main.js), so false short-circuits it entirely.
107+
'workbench.welcomePage.experimentalOnboarding': false,
104108
};
109+
/* eslint-enable @typescript-eslint/naming-convention */
105110

106111
// Output path
107112
const outputPath = `${__dirname}/../e2e-testing/test-ws/.vscode/settings.json`;

src/services/DhcService.spec.ts

Lines changed: 56 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest';
44
import { DhcService } from './DhcService';
55
import { ConfigService } from './ConfigService';
66
import type { RemoteFileSourceService } from './RemoteFileSourceService';
7-
import type { UniqueID } from '../types';
7+
import type { PublicOf, UniqueID } from '../types';
88

99
vi.mock('vscode');
1010

@@ -16,8 +16,23 @@ vi.mock('./ConfigService', () => {
1616
return { ConfigService: mockConfigService };
1717
});
1818

19+
beforeEach(() => {
20+
vi.clearAllMocks();
21+
});
22+
23+
function createMockCn(): DhcType.IdeConnection {
24+
return {
25+
getConsoleTypes: vi.fn().mockResolvedValue(['python']),
26+
} as unknown as DhcType.IdeConnection;
27+
}
28+
29+
type DhcServicePublicCn = PublicOf<DhcService> & {
30+
cn: DhcType.IdeConnection | null;
31+
};
32+
1933
/** Build a minimal DhcService instance that has a session already set up. */
2034
function createTestDhcService({
35+
cn = createMockCn(),
2136
pythonRemoteFileSourcePlugin,
2237
setControllerImportPrefixes = vi.fn(),
2338
setPythonServerExecutionContext = vi.fn().mockResolvedValue(undefined),
@@ -26,19 +41,16 @@ function createTestDhcService({
2641
changes: { created: [], updated: [], removed: [] },
2742
}),
2843
}: {
44+
cn?: DhcType.IdeConnection | null;
2945
pythonRemoteFileSourcePlugin?: DhcType.Widget | null;
3046
setControllerImportPrefixes?: ReturnType<typeof vi.fn>;
3147
setPythonServerExecutionContext?: ReturnType<typeof vi.fn>;
3248
sessionRunCode?: ReturnType<typeof vi.fn>;
33-
} = {}): DhcService {
49+
} = {}): DhcServicePublicCn {
3450
const mockSession = {
3551
runCode: sessionRunCode,
3652
} as unknown as DhcType.IdeSession;
3753

38-
const mockCn = {
39-
getConsoleTypes: vi.fn().mockResolvedValue(['python']),
40-
} as unknown as DhcType.IdeConnection;
41-
4254
const mockRemoteFileSourceService = {
4355
setControllerImportPrefixes,
4456
setPythonServerExecutionContext,
@@ -51,7 +63,7 @@ function createTestDhcService({
5163

5264
const service = Object.assign(Object.create(DhcService.prototype), {
5365
session: mockSession,
54-
cn: mockCn,
66+
cn,
5567
cnId: 'test-cn-id' as UniqueID,
5668
pythonRemoteFileSourcePlugin:
5769
pythonRemoteFileSourcePlugin !== undefined
@@ -82,13 +94,9 @@ function mockTextDoc(codeText: string): vscode.TextDocument {
8294
} as unknown as vscode.TextDocument;
8395
}
8496

85-
describe('DhcService.runCode – importPrefixes setting', () => {
97+
describe('runCode – importPrefixes setting', () => {
8698
const mockSetControllerImportPrefixes = vi.fn();
8799

88-
beforeEach(() => {
89-
vi.clearAllMocks();
90-
});
91-
92100
it.each([
93101
{
94102
label: 'uses setting prefixes when configured',
@@ -115,15 +123,17 @@ describe('DhcService.runCode – importPrefixes setting', () => {
115123
expected: null,
116124
},
117125
{
118-
label: 'calls with empty set for full file runs even when no prefixes found',
126+
label:
127+
'calls with empty set for full file runs even when no prefixes found',
119128
configPrefixes: undefined,
120129
input: mockTextDoc('x = 1'),
121130
expected: new Set(),
122131
},
123132
{
124133
label: 'setting takes precedence over meta_import in code',
125134
configPrefixes: ['forced'],
126-
input: 'deephaven_enterprise.controller_import.meta_import("otherPrefix")\n',
135+
input:
136+
'deephaven_enterprise.controller_import.meta_import("otherPrefix")\n',
127137
expected: new Set(['forced']),
128138
},
129139
{
@@ -150,3 +160,35 @@ describe('DhcService.runCode – importPrefixes setting', () => {
150160
}
151161
});
152162
});
163+
164+
describe('getConnection', () => {
165+
it('initializes the session when no connection exists yet', async () => {
166+
const service = createTestDhcService({ cn: null });
167+
168+
const mockCn = createMockCn();
169+
170+
// `initSession` establishes the connection as a side effect.
171+
const initSession = vi
172+
.spyOn(service, 'initSession')
173+
.mockImplementation(async () => {
174+
service.cn = mockCn;
175+
return true;
176+
});
177+
178+
const result = await service.getConnection();
179+
180+
expect(initSession).toHaveBeenCalledOnce();
181+
expect(result).toBe(mockCn);
182+
});
183+
184+
it('returns the existing connection without re-initializing', async () => {
185+
const service = createTestDhcService();
186+
187+
const initSession = vi.spyOn(service, 'initSession');
188+
189+
const result = await service.getConnection();
190+
191+
expect(initSession).not.toHaveBeenCalled();
192+
expect(result).toBe(service.cn);
193+
});
194+
});

src/services/DhcService.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,10 @@ export class DhcService extends DisposableBase implements IDhcService {
461461
}
462462

463463
async getConnection(): Promise<DhcType.IdeConnection | null> {
464+
if (this.cn == null) {
465+
await this.initSession();
466+
}
467+
464468
return this.cn;
465469
}
466470

src/services/ServerManager.spec.ts

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
import * as vscode from 'vscode';
2+
import { beforeEach, describe, expect, it, vi } from 'vitest';
3+
import { ServerManager } from './ServerManager';
4+
import { URLMap, withResolvers } from '../util';
5+
import type {
6+
ConnectionState,
7+
IAsyncCacheService,
8+
IConfigService,
9+
IDhcServiceFactory,
10+
IDheService,
11+
ISecretService,
12+
IToastService,
13+
PublicOf,
14+
ServerState,
15+
ServerType,
16+
} from '../types';
17+
18+
// See __mocks__/vscode.ts for the mock implementation
19+
vi.mock('vscode');
20+
21+
// Avoid real server status polling triggered by the constructor's
22+
// `loadServerConfig` call.
23+
vi.mock('../dh/dhc', () => ({
24+
isDhcServerRunning: vi.fn().mockResolvedValue(false),
25+
}));
26+
vi.mock('../dh/dhe', () => ({
27+
getWorkerCredentials: vi.fn(),
28+
isDheServerRunning: vi.fn().mockResolvedValue(false),
29+
}));
30+
31+
/**
32+
* Internal shape of `ServerManager` used to seed state and stub the actual
33+
* connection logic so we can exercise the `connectToServer` race-condition
34+
* handling in isolation.
35+
*/
36+
type TestServerManager = PublicOf<ServerManager> & {
37+
_serverMap: URLMap<ServerState>;
38+
_connectionMap: URLMap<ConnectionState>;
39+
_pendingConnectionMap: URLMap<Promise<ConnectionState | null>>;
40+
_doConnectToServer: ReturnType<typeof vi.fn>;
41+
};
42+
43+
/** Build a `ServerManager` with minimal mocked dependencies. */
44+
function createServerManager(): TestServerManager {
45+
const configService = {
46+
getCoreServers: vi.fn().mockReturnValue([]),
47+
getEnterpriseServers: vi.fn().mockReturnValue([]),
48+
} as unknown as IConfigService;
49+
50+
const dhcServiceFactory = {
51+
create: vi.fn(),
52+
} as unknown as IDhcServiceFactory;
53+
54+
const dheServiceCache = {
55+
has: vi.fn().mockReturnValue(false),
56+
get: vi.fn(),
57+
} as unknown as IAsyncCacheService<URL, IDheService>;
58+
59+
const manager = new ServerManager(
60+
configService,
61+
new URLMap(),
62+
dhcServiceFactory,
63+
new URLMap(),
64+
dheServiceCache,
65+
{ appendLine: vi.fn() } as unknown as vscode.OutputChannel,
66+
{} as ISecretService,
67+
{ info: vi.fn(), error: vi.fn() } as IToastService
68+
) as unknown as TestServerManager;
69+
70+
return manager;
71+
}
72+
73+
function mockConnectionState(url: URL): ConnectionState {
74+
return { isConnected: true, serverUrl: url };
75+
}
76+
77+
function mockServerState({
78+
url,
79+
type = 'DHC',
80+
connectionCount = 0,
81+
}: {
82+
url: URL;
83+
type?: ServerType;
84+
connectionCount?: number;
85+
}): ServerState {
86+
return {
87+
type,
88+
url,
89+
isConnected: connectionCount > 0,
90+
isRunning: true,
91+
connectionCount,
92+
};
93+
}
94+
95+
const serverUrl = new URL('http://localhost:10000/');
96+
const dhcServer0 = mockServerState({
97+
url: serverUrl,
98+
type: 'DHC',
99+
});
100+
const dhcServer1 = mockServerState({
101+
url: serverUrl,
102+
type: 'DHC',
103+
connectionCount: 1,
104+
});
105+
const dheServer0 = mockServerState({
106+
url: serverUrl,
107+
type: 'DHE',
108+
});
109+
const cn1 = mockConnectionState(serverUrl);
110+
const cn2 = mockConnectionState(serverUrl);
111+
112+
describe('ServerManager.connectToServer', () => {
113+
let manager: TestServerManager;
114+
115+
let promise: Promise<ConnectionState | null>;
116+
let resolve: (value: ConnectionState | null) => void;
117+
118+
beforeEach(() => {
119+
vi.clearAllMocks();
120+
manager = createServerManager();
121+
122+
({ promise, resolve } = withResolvers<ConnectionState | null>());
123+
124+
manager._doConnectToServer = vi.fn().mockReturnValue(promise);
125+
});
126+
127+
it('throws when the server is not found', async () => {
128+
await expect(manager.connectToServer(serverUrl)).rejects.toThrow(
129+
`Server with URL '${serverUrl}' not found.`
130+
);
131+
});
132+
133+
it('returns the existing connection for an already-connected DHC server', async () => {
134+
manager._serverMap.set(dhcServer1.url, dhcServer1);
135+
manager._connectionMap.set(serverUrl, cn1);
136+
137+
const result = await manager.connectToServer(serverUrl);
138+
139+
expect(result).toBe(cn1);
140+
expect(manager._doConnectToServer).not.toHaveBeenCalled();
141+
});
142+
143+
it('only connects once when called concurrently for the same DHC server', async () => {
144+
manager._serverMap.set(dhcServer0.url, dhcServer0);
145+
146+
const first = manager.connectToServer(serverUrl);
147+
const second = manager.connectToServer(serverUrl);
148+
149+
// The in-progress connection is reused rather than starting a new one.
150+
expect(manager._doConnectToServer).toHaveBeenCalledTimes(1);
151+
152+
resolve(cn1);
153+
154+
expect(await first).toBe(cn1);
155+
expect(await second).toBe(cn1);
156+
});
157+
158+
it('clears the pending connection once it resolves so later calls reconnect', async () => {
159+
manager._serverMap.set(dhcServer0.url, dhcServer0);
160+
161+
manager._doConnectToServer.mockResolvedValue(cn1);
162+
const first = manager.connectToServer(serverUrl);
163+
expect(manager._pendingConnectionMap.has(serverUrl)).toBe(true);
164+
expect(await first).toBe(cn1);
165+
166+
expect(manager._pendingConnectionMap.has(serverUrl)).toBe(false);
167+
168+
// A subsequent connect attempt is not blocked by the resolved pending entry.
169+
manager._doConnectToServer.mockResolvedValue(cn2);
170+
const second = manager.connectToServer(serverUrl);
171+
expect(await second).toBe(cn2);
172+
173+
expect(manager._doConnectToServer).toHaveBeenCalledTimes(2);
174+
});
175+
176+
it('does not track pending connections for DHE servers', async () => {
177+
manager._serverMap.set(dheServer0.url, dheServer0);
178+
179+
void manager.connectToServer(serverUrl);
180+
void manager.connectToServer(serverUrl);
181+
182+
// DHE supports multiple connections, so concurrent calls each start one.
183+
expect(manager._doConnectToServer).toHaveBeenCalledTimes(2);
184+
expect(manager._pendingConnectionMap.has(serverUrl)).toBe(false);
185+
});
186+
});

0 commit comments

Comments
 (0)