Skip to content

fix(completion): timeout + CancellationToken for pom IntelliSense network calls (#1127)#1170

Merged
wenytang-ms merged 5 commits into
mainfrom
fix/1127-completion-network-timeout
May 28, 2026
Merged

fix(completion): timeout + CancellationToken for pom IntelliSense network calls (#1127)#1170
wenytang-ms merged 5 commits into
mainfrom
fix/1127-completion-network-timeout

Conversation

@wenytang-ms

@wenytang-ms wenytang-ms commented May 27, 2026

Copy link
Copy Markdown
Contributor

What

Defensive hardening of the pom.xml IntelliSense completion path so that an unreachable / slow Maven Central no longer leaves the suggestions popup stuck on Loading... forever.

Refs #1127.

Why

In #1127 a user reports that Ctrl+Space brings up the suggestions popup but it stays on Loading... indefinitely. We've asked for more info to confirm the root cause (pom.xml vs Java file, network environment, logs), but regardless of the exact root cause in their setup, the existing completion code has a real "infinite Loading…" hazard worth fixing.

Tracing the pom completion provider:

  • src/extension.ts registers PomCompletionProvider for pom.xml with trigger characters ., -, <.
  • PomCompletionProvider.provideCompletionItems previously awaited each sub-provider serially and ignored the CancellationToken VS Code passes in.
  • The ArtifactProvider fan-out (Maven Central / index / local) was also fully serial: a slow source blocked all the others.
  • The HTTPS helper in src/utils/requestUtils.ts had:
    • no socket / request timeout — a stuck TCP connection (proxy, DNS black hole, unreachable Maven Central) left the Promise pending forever;
    • no request-level error handler, only a response-level one;
    • no awareness of CancellationToken.

Combined, that means one hung HTTPS call to search.maven.org is enough to make the completion popup stay on Loading... until VS Code is restarted — matching the symptom in #1127.

What changed

  • src/utils/requestUtils.ts
    • Adds a 10 s timeout (options.timeout + req.on('timeout')req.destroy(...)).
    • Adds a request-level error handler.
    • Adds a single settle guard so the Promise can only resolve or reject once.
    • Optionally accepts a vscode.CancellationToken and aborts the request via req.destroy() on cancellation.
    • Introduces a dedicated CancellationError class so that completion-side cancellations (which fire on every keystroke as VS Code supersedes the previous token) do not spam console.error. getArtifacts / getVersions suppress logging for that class only.
    • Re-checks the settled flag immediately after subscribing to token.onCancellationRequested, closing a tiny race where a synchronous error from https.get could leave the cancellation listener undisposed.
  • src/completion/PomCompletionProvider.ts
    • Honors the CancellationToken (early-out before parsing the doc, and after fan-out).
    • Runs the registered sub-providers concurrently with Promise.all + per-provider catch so one failing provider can no longer block the others.
  • src/completion/providers/ArtifactProvider.ts
    • Switches the central / index / local fan-out to Promise.allSettled: a network failure on Maven Central still returns local and index results.
    • Forwards the token to FromCentral (the only sub-provider that performs real network I/O — FromIndex is an in-process JDT.LS RPC and FromLocal is a fast-glob over ~/.m2/repository; neither has a public abort API).
  • src/completion/providers/IXmlCompletionProvider.ts / IArtifactProvider.ts / FromCentral.ts
    • Optional token parameter plumbing. Existing implementations remain compatible.

Behavior summary

  • Healthy network: same result set as before, sourced from all three providers. Tiny perf win because the three sources now run concurrently instead of serially.
  • Unreachable Maven Central: completion popup now resolves within ~10 s with whatever local / index results are available, instead of staying on Loading... indefinitely.
  • User cancels (types, moves cursor, presses Esc): in-flight HTTPS requests are aborted instead of running to completion.

Intentional behavior change in getArtifacts / getVersions

The existing try/catch in these helpers only wrapped JSON.parse. I widened the try to also wrap the await httpsGet(...) call. Net effect:

  • Before: a network-level rejection from httpsGet propagated out of getArtifacts / getVersions. The completion path tolerated this (it caught at a higher level), but the command-handler callers (src/handlers/addDependencyHandler.ts, src/handlers/setDependencyVersionHandler.ts) would surface an unhandled rejection.
  • After: any error (timeout, DNS failure, JSON parse failure) returns []. Command handlers now show an empty quick pick instead of failing, which is the safer UX when Maven Central is unreachable.

This is intentional but worth calling out for reviewers.

Tests

Added test/unit/requestUtils.test.ts (9 cases) covering the changed behavior. Tests stub https via proxyquire and drive the underlying state machine through data / end / error / timeout events:

Case Asserts
short keywords no HTTPS call, returns []
200 response parses docs
options.timeout equals 10 000 ms
pre-cancelled token short-circuits, no console.error
cancel mid-flight req.destroy called, no console.error
non-cancellation error (ECONNREFUSED) returns [], does log
socket timeout event destroys req with timeout error, logs
getVersions success parses docs
getVersions cancel silent, returns []

.mocharc.json also gains "node-option": ["no-experimental-strip-types"] to work around a Node 22.18+ default that caused newly created .test.ts files to be loaded by Mocha through Node's ESM strip-types path instead of the pre-existing ts-node/register/transpile-only CommonJS hook (so require() was undefined in the new file). With the flag set, all .test.ts files load uniformly via ts-node.

Validation

  • npm run compile — clean.
  • npm run tslint — no new warnings.
  • npm run test:unit30 passing (21 pre-existing + 9 new).
  • npm run webpack — bundles successfully.

Notes for reviewer

  • getLatestVersion and fetchPluginMetadataXml also use httpsGet and automatically get the new 10 s timeout. I intentionally did not change their public signatures (no caller passes a token today).
  • I considered making the 10 s timeout configurable via maven.completion.timeoutMs but kept it as a constant for now to minimize surface area; happy to expose it as a setting if reviewers want.
  • This change does not touch the JDT.LS-side Maven plugin (jdtls.ext/com.microsoft.java.maven.plugin). If Noticed IntelliSense stopped working #1127 turns out to be Java-side IntelliSense rather than pom.xml IntelliSense, the actual root cause likely lives there or in redhat.java / JDT.LS, and we'll need the additional info requested on the issue.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…telliSense network calls

The pom.xml completion provider (PomCompletionProvider) calls
search.maven.org via `src/utils/requestUtils.ts#httpsGet`. That helper had:

- no socket / request timeout, so a stuck TCP connection (corporate proxy,
  DNS black hole, unreachable Maven Central) would leave the underlying
  Promise pending forever;
- no request-level error handler, only a response-level one;
- no awareness of VS Code's `CancellationToken`, so even when the editor
  wanted to cancel a completion request the in-flight HTTPS call kept
  running.

Combined with the previous serial `await provider.provide(...)` loop in
`PomCompletionProvider.provideCompletionItems`, a single hung HTTPS call
made the entire IntelliSense popup stay on `Loading...` indefinitely
(see #1127).

This change:

- adds a 10s socket timeout to `httpsGet` and wires `req.on('timeout')`
  to `req.destroy(...)`;
- adds a request-level `error` handler and a single `settle` guard so
  the Promise can never resolve and reject;
- threads an optional `CancellationToken` through `getArtifacts` /
  `getVersions` down into `httpsGet` and aborts the request via
  `req.destroy()` on cancellation;
- in `PomCompletionProvider` honors the `CancellationToken` and runs
  the registered sub-providers concurrently with `Promise.all` + a
  per-provider try/catch, so a slow / failing provider can no longer
  block the others;
- in `ArtifactProvider` switches the central / index / local fan-out to
  `Promise.allSettled` so a network failure on Maven Central still
  returns local and index results instead of dropping everything.

No behavior change when the network is healthy: completions still come
from all three sources. When the network hangs or is unreachable, the
popup now resolves within 10s instead of getting stuck.

Refs #1127

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wenytang-ms and others added 2 commits May 27, 2026 13:55
Follow-up review fixes for #1127:

1. Introduce a dedicated CancellationError class. httpsGet now rejects
   with this class on token cancellation and request timeout cleanup,
   and getArtifacts/getVersions suppress console.error for it. VS Code
   cancels completion tokens on every keystroke, so without this every
   superseded request was logging "Error: Cancelled" to the extension
   output.

2. Document the (intentional) behavior change in getArtifacts /
   getVersions: the try/catch now wraps httpsGet too, so network errors
   are swallowed and return [] instead of propagating to command
   handlers like addDependencyHandler / setDependencyVersionHandler.
   This produces an empty quick pick on outage rather than an
   unhandled rejection, which is the safer UX. Noted in PR description.

3. Close a tiny race where cancelSub could be registered after the
   request had already errored synchronously, leaving the
   CancellationToken listener undisposed. We now re-check `settled`
   immediately after subscribing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the pom.xml IntelliSense completion pipeline so that slow/unreachable Maven Central (or other network pathologies) can’t leave VS Code completion stuck on Loading... indefinitely, by adding timeouts/cancellation and making provider fan-out resilient to partial failures.

Changes:

  • Add timeout, request-level error handling, and optional CancellationToken support to the HTTPS helper used for Maven Central queries.
  • Run pom completion sub-providers concurrently, while honoring VS Code cancellation and isolating failures so one provider can’t block the others.
  • Fan out Central/Index/Local artifact suggestions via Promise.allSettled so local/index results still appear when Central fails.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/utils/requestUtils.ts Adds HTTPS timeout, single-settle guarding, request error handling, and optional cancellation support for Maven Central calls.
src/completion/PomCompletionProvider.ts Runs completion providers concurrently and honors CancellationToken to avoid returning stale results.
src/completion/providers/IXmlCompletionProvider.ts Extends provider interface to optionally accept a cancellation token.
src/completion/providers/ArtifactProvider.ts Uses Promise.allSettled to aggregate Central/Index/Local results without one failure blocking all completions.
src/completion/providers/artifact/IArtifactProvider.ts Plumbs optional CancellationToken through artifact provider APIs.
src/completion/providers/artifact/FromCentral.ts Forwards token to Maven Central query helpers so in-flight requests can be aborted on cancellation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wenytang-ms and others added 2 commits May 27, 2026 14:51
Follow-up for PR #1170 (issue #1127). Adds 9 focused unit tests for
`getArtifacts` / `getVersions` that drive the underlying
`httpsGet` state machine through proxyquire-stubbed `https`:

- short-keyword short-circuit (no HTTPS call)
- successful 200 response → parsed docs
- `options.timeout` set to HTTPS_GET_TIMEOUT_MS (10s)
- pre-cancelled token → short-circuit, no console.error
- cancellation mid-flight → `req.destroy`, no console.error
- non-cancellation network error → returns [] AND logs
- socket idle `timeout` event → req destroyed with timeout error, logs
- `getVersions` success and silent cancellation paths

Also fixes the test harness so new `.test.ts` files load via
`ts-node/register/transpile-only` instead of Node's
`--experimental-strip-types` ESM loader (which is on by default in
Node 22.18+ and broke `require()` inside fresh test files).
The fix is one line in .mocharc.json:
`"node-option": ["no-experimental-strip-types"]`.

All 30 unit tests pass (21 pre-existing + 9 new).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI runs the unit tests on Node 20, which does not understand the
`--no-experimental-strip-types` flag I added in 88eba6a — the previous
commit broke `npm run test:unit` on all three CI runners with:
  bad option: --no-experimental-strip-types

Drop the unconditional `node-option` from .mocharc.json and wrap mocha
in scripts/run-unit-tests.js, which only forwards the flag on Node 22.6+
(where `--experimental-strip-types` actually exists and is the cause
of the harness quirk for newly added .test.ts files). Node 20 runs
mocha exactly as it did before this PR.

Verified locally on Node 22.21.1: 30 tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wenytang-ms

Copy link
Copy Markdown
Contributor Author

while in network error, while user don't cancel, there is no popup tooltip; after that, while timeout, the tooltip will show only local part and stop asking for httpget.
before
before1
before2
after
after

@wenytang-ms
wenytang-ms merged commit 9727226 into main May 28, 2026
47 of 48 checks passed
@wenytang-ms
wenytang-ms deleted the fix/1127-completion-network-timeout branch May 28, 2026 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants