Fix false offline detection and hanging web requests - #2783
Fix false offline detection and hanging web requests#2783PtJade-Ceramic wants to merge 2 commits into
Conversation
isOnline() now uses the system resolver (dns.lookup) instead of a c-ares Resolver, which can fail (e.g. ECONNREFUSED on Windows) and misreport the machine as offline even when online. The DNS timeout budget is corrected to ~1/50th of the install timeout with a 5s floor. Non-download requests now use a bounded 30s per-attempt timeout so a hung connection (e.g. one routed through a broken/partial proxy) fails quickly and falls back to the native fetch client, instead of blocking for the full user-configured timeout (default 600s). Adds unit tests for isOnline(). Fixes dotnet#2782
@dotnet-policy-service agree |
The 'Web Requests Cached Does Not Live Forever' test waited exactly the configured 120s TTL, but axios-cache-interceptor interprets the server's cache headers (default interpretHeader), so the entry's effective TTL is ~10 minutes (e.g. microsoft.com/example.com) and the 120s wait never observes expiry - making the test fail systematically on every platform. Adds an optional, backward-compatible 'requestOptions' parameter to getCachedData()/makeWebRequest() so callers (and tests) can override per-request axios options such as the cache TTL. The test now forces a 2s TTL with interpretHeader disabled, waits 3s, and asserts the entry expired - fast and deterministic. Also gives 'It actually times requests' an explicit timeout so it is robust on slow networks.
About the failing macOS checkThe macOS Build and Test failure is a pre-existing, repo-wide CI infrastructure issue — it is not caused by this PR. Evidence:
This is
So this PR's changes are green on the code itself; the macOS job is red due to the shared CI infrastructure. Happy to rebase/fix if the underlying macOS test-launch issue is addressed. |
106a7f5 to
5ba29ee
Compare
Summary
Fixes #2782: on machines where the c-ares DNS resolver fails but the system is online (e.g. VPN / proxy / hosts-file setups, common in some regions), the extension misreports the machine as offline ("It looks like you may be offline...") and installs fail after the full timeout with
DotnetOfflineFailure.The actual network was reachable (verified:
dns.lookupresolves, HTTPS tobuilds.dotnet.microsoft.comreturns 200) — the bug is in the offline-detection and request-timeout code, not the environment.Changes
vscode-dotnet-runtime-library/src/Utils/WebRequestWorkerSingleton.ts1.
isOnline()no longer uses a c-aresResolverdns.promises.Resolver(c-ares, reads a static DNS server list) withdns.promises.lookup, which uses the same resolution path as real socket connections (hosts file, VPN adapters, system DNS).timeoutSec * 2ms (1.2s at the default 600s — 10× shorter than the original comment's "1/50th of the download time") toMath.max(timeoutSec * 20, 5000)(12s at default, 5s floor).OfflineDetectionLogicTriggeredand returnfalse, so genuinely-offline behavior is unchanged.resolveHostnameForOnlineCheck()so the resolver is testable.2. Hung requests fail fast instead of blocking for the full timeout
attemptTimeoutMs), so a connection that hangs — e.g. one routed through a broken/partial proxy — fails quickly and triggers the existing native fetch (no-proxy) fallback, instead of blocking for the full user-configured timeout (default 600s).3.
getCachedData()/makeWebRequest()accept optional per-request axios options (backward-compatible) so callers can override e.g. the cache TTL. Used by the fixed test below.Tests
vscode-dotnet-runtime-library/src/test/unit/WebRequestWorker.test.ts:isOnline()unit tests (resolves / does not resolve / DNS error /DOTNET_INSTALL_TOOL_OFFLINE).Web Requests Cached Does Not Live Forevertest: it waited exactly the configured 120s TTL, but axios-cache-interceptor interprets the server's cache headers (defaultinterpretHeader: true), so the entry's effective TTL is ~10 minutes (e.g.microsoft.com/example.comreturn a longmax-age) and the 120s wait never observed expiry — failing on every platform. The test now forces a 2s TTL withinterpretHeaderdisabled, waits 3s, and asserts the entry expired: fast and deterministic.It actually times requestsan explicit timeout so it is robust on slow networks.vscode-dotnet-runtime-library/src/test/mocks/MockObjects.ts:MockTrackingWebRequestWorkerforwards the new optional request options.Changelog
[Unreleased]entries added to both the runtime and SDK extension CHANGELOGs.Validation
npm run compilepasses.WebRequestWorkerunit tests pass locally (11/11), including the previously-failing cache-expiry test and the newisOnline()tests;LocalInstallUpdateService(11/11) also passes.Resolver→ECONNREFUSEDforwww.microsoft.com, whiledns.lookupresolves in ~26ms and HTTPS tobuilds.dotnet.microsoft.comreturns HTTP 200.Notes
get-proxy-settingsdependency could be replaced with a maintained alternative, and installer downloads could fail over to alternate Microsoft CDNs.