Skip to content

Commit 9b165fd

Browse files
committed
Merge remote-tracking branch 'upstream/main' into fix/vscode-e2e-teardown
2 parents a5b7ffe + df6e3a5 commit 9b165fd

5 files changed

Lines changed: 353 additions & 9 deletions

File tree

‎vscode/extension/src/extension.ts‎

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@ import {
2424
traceError,
2525
} from './utilities/common/log'
2626
import { onDidChangePythonInterpreter } from './utilities/common/python'
27+
import { requiresLspRestart } from './utilities/common/configurationChange'
28+
import { coalesceAsync } from './utilities/coalesceAsync'
2729
import { sleep } from './utilities/sleep'
28-
import { handleError } from './utilities/errors'
30+
import { ErrorType, handleError } from './utilities/errors'
2931

3032
import { selector, completionProvider } from './completion/completion'
3133
import { LineagePanel } from './webviews/lineagePanel'
@@ -65,20 +67,32 @@ export async function activate(context: vscode.ExtensionContext) {
6567
),
6668
)
6769

68-
const restartLsp = async (invokedByUser = false): Promise<void> => {
70+
/**
71+
* Set when a restart was asked for explicitly, so that a user-invoked restart
72+
* coalesced together with an automatic one is still treated as user-invoked.
73+
*/
74+
let restartInvokedByUser = false
75+
76+
/**
77+
* Set by a failed run and handled by the caller once the run has finished.
78+
* Handling it inside the run would deadlock: the not_signed_in handler waits
79+
* on a sign-in that restarts the client again, and that restart would wait on
80+
* the run that is still waiting on the handler.
81+
*/
82+
let restartError: ErrorType | undefined
83+
84+
const runRestart = async (): Promise<void> => {
85+
const invokedByUser = restartInvokedByUser
86+
restartInvokedByUser = false
87+
6988
if (!lspClient) {
7089
lspClient = new LSPClient()
7190
}
7291

7392
traceVerbose('Restarting SQLMesh LSP client')
7493
const result = await lspClient.restart(invokedByUser)
7594
if (isErr(result)) {
76-
await handleError(
77-
authProvider,
78-
restartLsp,
79-
result.error,
80-
'LSP restart failed',
81-
)
95+
restartError = result.error
8296
return
8397
}
8498

@@ -95,6 +109,25 @@ export async function activate(context: vscode.ExtensionContext) {
95109
context.subscriptions.push(testControllerDisposable)
96110
}
97111

112+
/**
113+
* Restarts are serialized: a client disposing while the next one starts up
114+
* leaves colliding command registrations and requests aimed at a disposed
115+
* client, which is what surfaced as constant crashes.
116+
*/
117+
const restartLspSerialized = coalesceAsync(runRestart)
118+
119+
const restartLsp = async (invokedByUser = false): Promise<void> => {
120+
restartInvokedByUser = restartInvokedByUser || invokedByUser
121+
await restartLspSerialized()
122+
123+
// Claimed so that callers coalesced into the same run don't each report it.
124+
const error = restartError
125+
restartError = undefined
126+
if (error) {
127+
await handleError(authProvider, restartLsp, error, 'LSP restart failed')
128+
}
129+
}
130+
98131
// commands needing the restart helper
99132
context.subscriptions.push(
100133
vscode.commands.registerCommand(
@@ -191,7 +224,13 @@ export async function activate(context: vscode.ExtensionContext) {
191224

192225
context.subscriptions.push(
193226
onDidChangePythonInterpreter(() => restartLsp()),
194-
onDidChangeConfiguration(() => restartLsp()),
227+
// Only restart for settings the server actually reads. This event fires for
228+
// every setting in the editor, including ones written by other extensions.
229+
onDidChangeConfiguration(event => {
230+
if (requiresLspRestart(event)) {
231+
void restartLsp()
232+
}
233+
}),
195234
)
196235

197236
if (!lspClient.hasCompletionCapability()) {
Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
3+
import { describe, expect, it } from 'vitest'
4+
import { coalesceAsync } from './coalesceAsync'
5+
6+
/**
7+
* Let pending microtasks run. A rerun is scheduled off the promise of the run
8+
* that precedes it, so it starts a few hops after that run settles.
9+
*/
10+
const flush = () => new Promise<void>(resolve => setTimeout(resolve, 0))
11+
12+
/** A task that only settles when the test releases it. */
13+
function deferredTask() {
14+
let release: (() => void) | undefined
15+
let rejectWith: ((error: Error) => void) | undefined
16+
let calls = 0
17+
18+
const run = () => {
19+
calls += 1
20+
return new Promise<void>((resolve, reject) => {
21+
release = resolve
22+
rejectWith = reject
23+
})
24+
}
25+
26+
return {
27+
run,
28+
get calls() {
29+
return calls
30+
},
31+
release: () => release?.(),
32+
reject: (error: Error) => rejectWith?.(error),
33+
}
34+
}
35+
36+
describe('coalesceAsync', () => {
37+
it('runs the task immediately when idle', async () => {
38+
const task = deferredTask()
39+
const run = coalesceAsync(task.run)
40+
41+
const first = run()
42+
expect(task.calls).toBe(1)
43+
44+
task.release()
45+
await first
46+
})
47+
48+
// A burst of triggers used to start a restart per event, which left clients
49+
// disposing and starting concurrently. See #5642.
50+
it('collapses every call made while running into a single rerun', async () => {
51+
const task = deferredTask()
52+
const run = coalesceAsync(task.run)
53+
54+
const first = run()
55+
expect(task.calls).toBe(1)
56+
57+
const queued = [run(), run(), run(), run()]
58+
expect(task.calls).toBe(1)
59+
60+
task.release()
61+
await first
62+
await flush()
63+
64+
// Exactly one rerun is scheduled, no matter how many calls arrived.
65+
expect(task.calls).toBe(2)
66+
67+
task.release()
68+
await Promise.all(queued)
69+
await flush()
70+
expect(task.calls).toBe(2)
71+
})
72+
73+
it('runs again for calls made after the previous run finished', async () => {
74+
const task = deferredTask()
75+
const run = coalesceAsync(task.run)
76+
77+
const first = run()
78+
task.release()
79+
await first
80+
expect(task.calls).toBe(1)
81+
82+
const second = run()
83+
expect(task.calls).toBe(2)
84+
task.release()
85+
await second
86+
})
87+
88+
it('resolves the callers that were coalesced together', async () => {
89+
const task = deferredTask()
90+
const run = coalesceAsync(task.run)
91+
92+
const first = run()
93+
const queued = [run(), run()]
94+
95+
task.release()
96+
await first
97+
// The rerun has to be in flight before it can be released.
98+
await flush()
99+
task.release()
100+
101+
await expect(Promise.all(queued)).resolves.toEqual([undefined, undefined])
102+
})
103+
104+
it('surfaces a failure without wedging later calls', async () => {
105+
const task = deferredTask()
106+
const run = coalesceAsync(task.run)
107+
108+
const first = run()
109+
task.reject(new Error('restart failed'))
110+
await expect(first).rejects.toThrow('restart failed')
111+
112+
const second = run()
113+
expect(task.calls).toBe(2)
114+
task.release()
115+
await second
116+
})
117+
118+
// A failed restart is reported to the caller, which handles it after the run
119+
// has finished. The not_signed_in handler signs the user in and restarts the
120+
// client again, so that handler must not run inside the task: the restart it
121+
// triggers would wait on the run that is still waiting on the handler, and
122+
// neither would ever settle. See #5920.
123+
it('lets a failure handler trigger another run without deadlocking', async () => {
124+
let failNextRun = true
125+
let reportedError: string | undefined
126+
let runs = 0
127+
128+
const runRestart = async (): Promise<void> => {
129+
runs += 1
130+
// The real run awaits the client restart before it knows the outcome.
131+
// Without that await the re-entrant call below lands in the synchronous
132+
// prefix of the run, before it has been recorded as in flight, and the
133+
// deadlock this covers cannot happen.
134+
const failed = await Promise.resolve(failNextRun)
135+
if (failed) {
136+
failNextRun = false
137+
reportedError = 'not_signed_in'
138+
}
139+
}
140+
141+
const runRestartSerialized = coalesceAsync(runRestart)
142+
143+
const restart = async (): Promise<void> => {
144+
await runRestartSerialized()
145+
146+
const error = reportedError
147+
reportedError = undefined
148+
if (error === 'not_signed_in') {
149+
// Stands in for the sign-in flow, which restarts once signed in.
150+
await restart()
151+
}
152+
}
153+
154+
await restart()
155+
156+
expect(runs).toBe(2)
157+
expect(reportedError).toBeUndefined()
158+
}, 2_000)
159+
})
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
3+
/**
4+
* Serialize an async task so it never overlaps with itself, collapsing any
5+
* calls that arrive while it is running into a single rerun.
6+
*
7+
* Restarting the language server means stopping a client and starting another.
8+
* Letting two of those interleave leaves commands registered by the outgoing
9+
* client colliding with the incoming one, and leaves requests addressed to a
10+
* client that has already been disposed. Queueing one run per trigger would
11+
* only spread the same problem out over time, so queued triggers collapse into
12+
* one rerun: all the caller wants is for the task to have run after its
13+
* request, not for it to run once per request.
14+
*
15+
* @param task The task to serialize.
16+
* @returns A function that resolves once the task has run for that call.
17+
*/
18+
export function coalesceAsync(task: () => Promise<void>): () => Promise<void> {
19+
let running: Promise<void> | undefined
20+
let queued: Promise<void> | undefined
21+
22+
const start = async (): Promise<void> => {
23+
try {
24+
await task()
25+
} finally {
26+
running = undefined
27+
}
28+
}
29+
30+
return (): Promise<void> => {
31+
if (!running) {
32+
running = start()
33+
return running
34+
}
35+
36+
// A rerun is already scheduled, so this call is satisfied by that one.
37+
if (!queued) {
38+
queued = running
39+
// A failed run must not stop the rerun that was asked for; the caller
40+
// waiting on the failed run is the one that sees the error.
41+
.catch(() => undefined)
42+
.then(() => {
43+
queued = undefined
44+
running = start()
45+
return running
46+
})
47+
}
48+
49+
return queued
50+
}
51+
}
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
3+
import { describe, expect, it } from 'vitest'
4+
import { requiresLspRestart } from './configurationChange'
5+
6+
/**
7+
* Build a stand-in for `vscode.ConfigurationChangeEvent` from the settings that
8+
* changed. VS Code reports a section as affected when the changed key is the
9+
* section itself or sits underneath it.
10+
*/
11+
const changed = (...keys: string[]) => ({
12+
affectsConfiguration: (section: string) =>
13+
keys.some(key => key === section || key.startsWith(`${section}.`)),
14+
})
15+
16+
describe('requiresLspRestart', () => {
17+
it('restarts when a sqlmesh setting changes', () => {
18+
expect(requiresLspRestart(changed('sqlmesh.projectPaths'))).toBe(true)
19+
expect(requiresLspRestart(changed('sqlmesh.lspEntrypoint'))).toBe(true)
20+
})
21+
22+
it('restarts when the python interpreter changes', () => {
23+
expect(requiresLspRestart(changed('python.defaultInterpreterPath'))).toBe(
24+
true,
25+
)
26+
})
27+
28+
// The LSP used to restart on every configuration change in the editor, so
29+
// anything that wrote a setting took the extension down with it. See #5920.
30+
it('ignores settings the language server does not read', () => {
31+
expect(requiresLspRestart(changed('editor.fontSize'))).toBe(false)
32+
expect(requiresLspRestart(changed('workbench.colorTheme'))).toBe(false)
33+
expect(requiresLspRestart(changed('files.autoSave'))).toBe(false)
34+
})
35+
36+
// Running any python command in a VS Code terminal makes the Python
37+
// extension touch its own terminal settings, which is what made the
38+
// extension crash whenever sqlmesh was run in the terminal. See #5642.
39+
it('ignores python settings unrelated to the interpreter', () => {
40+
expect(
41+
requiresLspRestart(changed('python.terminal.activateEnvironment')),
42+
).toBe(false)
43+
expect(
44+
requiresLspRestart(changed('python.analysis.typeCheckingMode')),
45+
).toBe(false)
46+
expect(requiresLspRestart(changed('terminal.integrated.env.linux'))).toBe(
47+
false,
48+
)
49+
})
50+
51+
it('does not restart when nothing relevant changed', () => {
52+
expect(requiresLspRestart(changed())).toBe(false)
53+
})
54+
55+
it('restarts when a relevant setting changes alongside irrelevant ones', () => {
56+
expect(
57+
requiresLspRestart(changed('editor.fontSize', 'sqlmesh.projectPaths')),
58+
).toBe(true)
59+
})
60+
})

0 commit comments

Comments
 (0)