Skip to content

Commit ab87849

Browse files
committed
fix(lsp): jump-to-definition hung forever when the server had no answer
The intermittent CI failure of "should jump to definition" (LegacyInteg: EditorCommandHandlers) was not cold-start latency: the spec's first NAVIGATE_JUMPTO_DEFINITION fired while tsserver was still loading the project, the server answered with no definition, and doJumpToDef's deferred only resolved inside `if (msgObj && msgObj.range)` - an empty result NEVER settled it. The command promise then hung forever, and awaitsFor races each poll invocation against the FULL timeout, so the "retry loop" never retried once and the spec burned its whole budget on one dead promise (the SpecRunner.js:141 stack in the CI logs). Manual jumps worked the whole time - the server was healthy; the spec was pinned. - DefaultProviders.doJumpToDef: settle (reject) when the server returns no definition, and on a failed cross-file FILE_OPEN. Also fixes real users: F12 on a symbol with no definition left a permanently pending command. - src-node/lsp-client: on unexpected server exit, reject all in-flight requests immediately - the dead process can never answer them, and they previously hung callers until the 2-minute per-request timeout (only graceful stops flushed the pending map). - The spec: the one-time tsserver cold start now happens in the suite's beforeAll warm-up (the TypeScript LSP suite's pattern, 90s budget there); the spec itself waits max 15s, polling every 500ms with each jump attempt time-boxed to 3s - a slow or rejected attempt is a retry, never a pin - and a rejected jump no longer aborts awaitsFor via an exception. LegacyInteg:EditorCommandHandlers Integration green 12/12 twice in a row (including the focus-sensitive clipboard spec).
1 parent 92cb4e8 commit ab87849

3 files changed

Lines changed: 58 additions & 7 deletions

File tree

src-node/lsp-client.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,12 @@ exports.startServer = async function startServer(params) {
218218
if (code) {
219219
console.error(`[lsp-client][${serverId}] exited code=${code} signal=${signal || 'none'}`);
220220
}
221+
// Fail every in-flight request immediately - the dead process can never answer them, and
222+
// leaving them pending stalls callers until the per-request timeout (2 minutes).
223+
for (const { reject: rejectPending } of serverState.pending.values()) {
224+
rejectPending(new Error(`Server ${serverId} exited with pending request`));
225+
}
226+
serverState.pending.clear();
221227
nodeConnector.triggerPeer('serverExit', { serverId, code, signal, stderr });
222228
if (!hasResolved) {
223229
hasResolved = true;

src/languageTools/DefaultProviders.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -814,11 +814,19 @@ define(function (require, exports, module) {
814814
.done(function () {
815815
setJumpPosition(startCurPos);
816816
$deferredHints.resolve();
817+
})
818+
.fail(function () {
819+
$deferredHints.reject();
817820
});
818821
} else { //definition is in current document
819822
setJumpPosition(startCurPos);
820823
$deferredHints.resolve();
821824
}
825+
} else {
826+
// No definition at this position (servers answer null/[] - e.g. tsserver while it
827+
// is still loading the project). MUST settle: an unresolved deferred here leaves
828+
// the NAVIGATE_JUMPTO_DEFINITION command promise pending forever.
829+
$deferredHints.reject();
822830
}
823831
}).fail(function () {
824832
$deferredHints.reject();

test/spec/EditorCommandHandlers-integ-test.js

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,27 @@ define(function (require, exports, module) {
115115
EditorManager = testWindow.brackets.test.EditorManager;
116116

117117
await SpecRunnerUtils.loadProjectInTestWindow(testPath);
118-
}, 30000);
118+
119+
if (testWindow.Phoenix.isNativeApp) {
120+
// Warm up the TypeScript language server here, where the one-time cold start (spawn
121+
// vtsls + tsserver loading the TypeScript library and project) belongs - so the
122+
// jump-to-definition spec below only needs a short wait for its own request instead
123+
// of budgeting for a cold server (its very first request could otherwise pend longer
124+
// than any reasonable spec timeout on slow/loaded CI runners).
125+
await awaitsForDone(
126+
CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN,
127+
{fullPath: testPath + "/test.js"}),
128+
"warm-up: open test.js");
129+
const LSPClient = await new Promise(function (resolve) {
130+
testWindow.brackets.getModule(["languageTools/LSPClient"], resolve);
131+
});
132+
await awaitsFor(function () {
133+
return LSPClient.isLintingProviderActive("javascript");
134+
}, "the TypeScript language server to finish its cold start", 90000);
135+
await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE_ALL, { _forceClose: true }),
136+
"close warm-up file");
137+
}
138+
}, 120000);
119139

120140
afterAll(async function () {
121141
await closeTestWindow();
@@ -202,17 +222,34 @@ define(function (require, exports, module) {
202222
// JavaScript at a higher priority than the built-in Tern provider. vtsls returns
203223
// testMe's full declaration range and we jump to its start (the `function`
204224
// keyword), giving a collapsed cursor at {0,0} - whereas Tern selects the
205-
// identifier name. The server can take a moment to prime after the file opens, so
206-
// retry the jump until the LSP result lands rather than using a fixed wait (which
207-
// flakes on slow CI: before vtsls is ready, Tern answers with {0,9}-{0,15}).
225+
// identifier name. The server was already warmed up in beforeAll, so 15s is
226+
// plenty here. Each attempt is time-boxed: a slow or unanswered request counts
227+
// as a retry instead of pinning the whole wait (awaitsFor races each pollFn
228+
// invocation against the FULL timeout, so one hung attempt would otherwise eat
229+
// the entire budget), and a rejected jump ("no definition yet") also retries -
230+
// awaitsFor aborts outright on a pollFn exception, so the promise is absorbed.
208231
await awaitsFor(async function () {
209232
myEditor.setCursorPos({line: 5, ch: 8});
210-
await awaitsForDone(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION),
211-
"Jump To Definition");
233+
var landed = await new Promise(function (resolve) {
234+
var settled = false;
235+
function settle(val) {
236+
if (!settled) {
237+
settled = true;
238+
resolve(val);
239+
}
240+
}
241+
CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION)
242+
.done(function () { settle(true); })
243+
.fail(function () { settle(false); });
244+
setTimeout(function () { settle(false); }, 3000);
245+
});
246+
if (!landed) {
247+
return false;
248+
}
212249
var sel = myEditor.getSelection();
213250
return sel.start.line === 0 && sel.start.ch === 0 &&
214251
sel.end.line === 0 && sel.end.ch === 0;
215-
}, "LSP jump-to-definition to land on the testMe declaration", 30000);
252+
}, "LSP jump-to-definition to land on the testMe declaration", 15000, 500);
216253
selection = myEditor.getSelection();
217254
expect(fixSel(selection)).toEql(fixSel({
218255
start: {line: 0, ch: 0},

0 commit comments

Comments
 (0)