-
-
Notifications
You must be signed in to change notification settings - Fork 856
fix(vscode): Prevent timeout in binary search for large pnpm monorepos #17580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||||||||||||||
| import * as fs from "node:fs"; | ||||||||||||||||||||||||||||
| import * as path from "node:path"; | ||||||||||||||||||||||||||||
| import { ConfigurationChangeEvent, RelativePattern, Uri, workspace, WorkspaceFolder } from "vscode"; | ||||||||||||||||||||||||||||
| import { DiagnosticPullMode } from "vscode-languageclient"; | ||||||||||||||||||||||||||||
|
|
@@ -18,6 +19,9 @@ export class ConfigService implements IDisposable { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private workspaceConfigs: Map<string, WorkspaceConfig> = new Map(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Cache for binary path lookups to avoid repeated expensive searches | ||||||||||||||||||||||||||||
| private binaryPathCache: Map<string, string | undefined> = new Map(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public onConfigChange: | ||||||||||||||||||||||||||||
| | ((this: ConfigService, config: ConfigurationChangeEvent) => Promise<void>) | ||||||||||||||||||||||||||||
| | undefined; | ||||||||||||||||||||||||||||
|
|
@@ -64,10 +68,14 @@ export class ConfigService implements IDisposable { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public addWorkspaceConfig(workspace: WorkspaceFolder): void { | ||||||||||||||||||||||||||||
| this.workspaceConfigs.set(workspace.uri.path, new WorkspaceConfig(workspace)); | ||||||||||||||||||||||||||||
| // Invalidate cache when workspace folders change | ||||||||||||||||||||||||||||
| this.binaryPathCache.clear(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public removeWorkspaceConfig(workspace: WorkspaceFolder): void { | ||||||||||||||||||||||||||||
| this.workspaceConfigs.delete(workspace.uri.path); | ||||||||||||||||||||||||||||
| // Invalidate cache when workspace folders change | ||||||||||||||||||||||||||||
| this.binaryPathCache.clear(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public getWorkspaceConfig(workspace: Uri): WorkspaceConfig | undefined { | ||||||||||||||||||||||||||||
|
|
@@ -109,6 +117,117 @@ export class ConfigService implements IDisposable { | |||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Checks if a binary exists at the given path. | ||||||||||||||||||||||||||||
| * Handles both Unix and Windows path differences. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private checkBinaryExists(binaryPath: string): boolean { | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| // Check if file exists and is executable (on Unix) or accessible (on Windows) | ||||||||||||||||||||||||||||
| fs.accessSync(binaryPath, fs.constants.F_OK); | ||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Searches for a binary in node_modules/.bin directories using file system operations. | ||||||||||||||||||||||||||||
| * This is much faster than workspace.findFiles in large monorepos. | ||||||||||||||||||||||||||||
| * Limits search depth and scope to avoid timeouts. | ||||||||||||||||||||||||||||
| * Note: This method searches upward from the start path toward the filesystem root. | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not like this too. We have |
||||||||||||||||||||||||||||
| * For nested subdirectories within the workspace, the findFiles fallback handles those cases. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private async searchBinaryInNodeModules( | ||||||||||||||||||||||||||||
| startPath: string, | ||||||||||||||||||||||||||||
| binaryName: string, | ||||||||||||||||||||||||||||
| maxDepth: number = 3, | ||||||||||||||||||||||||||||
| currentDepth: number = 0, | ||||||||||||||||||||||||||||
| ): Promise<string | undefined> { | ||||||||||||||||||||||||||||
| // Limit depth to avoid deep recursion in large monorepos | ||||||||||||||||||||||||||||
| if (currentDepth >= maxDepth) { | ||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const nodeModulesPath = path.join(startPath, "node_modules", ".bin"); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Skip .pnpm directories to avoid traversing into pnpm's internal structure | ||||||||||||||||||||||||||||
| // Check if we're inside a .pnpm directory path | ||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not like it, that we have a special check for only |
||||||||||||||||||||||||||||
| startPath.includes(path.sep + ".pnpm" + path.sep) || | ||||||||||||||||||||||||||||
| startPath.endsWith(path.sep + ".pnpm") | ||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||
| // Skip traversing into .pnpm directories - go directly to parent | ||||||||||||||||||||||||||||
| const parentPath = path.dirname(startPath); | ||||||||||||||||||||||||||||
| if (parentPath === startPath || parentPath === "/" || parentPath.match(/^[A-Za-z]:\\?$/i)) { | ||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return this.searchBinaryInNodeModules(parentPath, binaryName, maxDepth, currentDepth + 1); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Check if this .bin directory exists | ||||||||||||||||||||||||||||
| if (fs.existsSync(nodeModulesPath)) { | ||||||||||||||||||||||||||||
| const binaryPath = path.join(nodeModulesPath, binaryName); | ||||||||||||||||||||||||||||
| if (this.checkBinaryExists(binaryPath)) { | ||||||||||||||||||||||||||||
| return binaryPath; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Check with .exe extension on Windows | ||||||||||||||||||||||||||||
| if (process.platform === "win32") { | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For windows, we do not use the oxc/editors/vscode/client/tools/lsp_helper.ts Lines 26 to 38 in f02c0e7
|
||||||||||||||||||||||||||||
| const exePath = `${binaryPath}.exe`; | ||||||||||||||||||||||||||||
| if (this.checkBinaryExists(exePath)) { | ||||||||||||||||||||||||||||
| return exePath; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Search in parent directories starting at the workspace root toward the filesystem root | ||||||||||||||||||||||||||||
| const parentPath = path.dirname(startPath); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Stop if we've reached the filesystem root or if parent is same as current | ||||||||||||||||||||||||||||
| if (parentPath === startPath || parentPath === "/" || parentPath.match(/^[A-Za-z]:\\?$/i)) { | ||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Recursively search parent directory | ||||||||||||||||||||||||||||
| return this.searchBinaryInNodeModules(parentPath, binaryName, maxDepth, currentDepth + 1); | ||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||
| // Ignore errors (permission denied, etc.) and continue searching | ||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Wraps workspace.findFiles with a timeout to prevent indefinite hanging. | ||||||||||||||||||||||||||||
| * Returns empty array if the search times out or fails. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| private async findFilesWithTimeout( | ||||||||||||||||||||||||||||
| pattern: RelativePattern, | ||||||||||||||||||||||||||||
| exclude: string | null, | ||||||||||||||||||||||||||||
| maxResults: number, | ||||||||||||||||||||||||||||
| timeoutMs: number = 5000, | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know of some cases (mainly networked drives) where searching for files could take at least a few seconds, and so we may want to consider bumping this up a bit. We could make it configurable, but it's not my preference. |
||||||||||||||||||||||||||||
| ): Promise<Uri[]> { | ||||||||||||||||||||||||||||
| const searchPromise = workspace.findFiles(pattern, exclude, maxResults); | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||||||||||||||||||||||||||||
| const timeoutPromise = new Promise<Uri[]>((_, reject) => { | ||||||||||||||||||||||||||||
| timeoutId = setTimeout(() => reject(new Error("Binary search timeout")), timeoutMs); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| return await Promise.race([searchPromise, timeoutPromise]); | ||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||
| // Timeout or other error - return empty array to indicate failure | ||||||||||||||||||||||||||||
| // This allows graceful degradation without throwing | ||||||||||||||||||||||||||||
| return []; | ||||||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||||||
| // Clear timeout to prevent resource leak | ||||||||||||||||||||||||||||
| if (timeoutId !== undefined) { | ||||||||||||||||||||||||||||
| clearTimeout(timeoutId); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private async searchBinaryPath( | ||||||||||||||||||||||||||||
| settingsBinary: string | undefined, | ||||||||||||||||||||||||||||
| defaultPattern: string, | ||||||||||||||||||||||||||||
|
|
@@ -119,14 +238,72 @@ export class ConfigService implements IDisposable { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (!settingsBinary) { | ||||||||||||||||||||||||||||
| // try to find the binary in node_modules/.bin, resolve to the first workspace folder | ||||||||||||||||||||||||||||
| const files = await workspace.findFiles( | ||||||||||||||||||||||||||||
| new RelativePattern(cwd, `**/node_modules/.bin/${defaultPattern}`), | ||||||||||||||||||||||||||||
| null, | ||||||||||||||||||||||||||||
| 1, | ||||||||||||||||||||||||||||
| // Check cache first | ||||||||||||||||||||||||||||
| const cacheKey = `auto:${defaultPattern}`; | ||||||||||||||||||||||||||||
| if (this.binaryPathCache.has(cacheKey)) { | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||
| return this.binaryPathCache.get(cacheKey); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Fast path: Check common locations first using file system operations | ||||||||||||||||||||||||||||
| // This is much faster than recursive findFiles in large monorepos | ||||||||||||||||||||||||||||
| const workspaceFolders = Array.from(this.workspaceConfigs.keys()); | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old implementation used only the first workspace folder. I guess this could lead to performance problems too. Specially with setups like in this example: |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Step 1: Check root node_modules/.bin in each workspace folder | ||||||||||||||||||||||||||||
| for (const workspacePath of workspaceFolders) { | ||||||||||||||||||||||||||||
| const rootBinPath = path.join(workspacePath, "node_modules", ".bin", defaultPattern); | ||||||||||||||||||||||||||||
| if (this.checkBinaryExists(rootBinPath)) { | ||||||||||||||||||||||||||||
| this.binaryPathCache.set(cacheKey, rootBinPath); | ||||||||||||||||||||||||||||
| return rootBinPath; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Also check with .exe extension on Windows | ||||||||||||||||||||||||||||
| if (process.platform === "win32") { | ||||||||||||||||||||||||||||
| const exePath = `${rootBinPath}.exe`; | ||||||||||||||||||||||||||||
| if (this.checkBinaryExists(exePath)) { | ||||||||||||||||||||||||||||
| this.binaryPathCache.set(cacheKey, exePath); | ||||||||||||||||||||||||||||
| return exePath; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Step 2: Search parent directories starting at the workspace root toward the filesystem root using file system operations | ||||||||||||||||||||||||||||
| // This handles cases where binaries are in nested or ancestor packages | ||||||||||||||||||||||||||||
| // Search all workspace folders in parallel for better performance | ||||||||||||||||||||||||||||
| const searchPromises = workspaceFolders.map((workspacePath) => | ||||||||||||||||||||||||||||
| this.searchBinaryInNodeModules(workspacePath, defaultPattern, 3), | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| const searchResults = await Promise.all(searchPromises); | ||||||||||||||||||||||||||||
| const found = searchResults.find((result) => result !== undefined); | ||||||||||||||||||||||||||||
| if (found) { | ||||||||||||||||||||||||||||
| this.binaryPathCache.set(cacheKey, found); | ||||||||||||||||||||||||||||
| return found; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return files.length > 0 ? files[0].fsPath : undefined; | ||||||||||||||||||||||||||||
| // Step 3: Last resort - use findFiles with timeout protection | ||||||||||||||||||||||||||||
| // Search recursively within workspace folders, excluding .pnpm-related directories | ||||||||||||||||||||||||||||
| // Use Promise.race to add timeout protection | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const excludePattern = "**/{.pnpm,node_modules/.pnpm}/**"; | ||||||||||||||||||||||||||||
| const files = await this.findFilesWithTimeout( | ||||||||||||||||||||||||||||
| new RelativePattern(cwd, `**/node_modules/.bin/${defaultPattern}`), | ||||||||||||||||||||||||||||
| excludePattern, | ||||||||||||||||||||||||||||
| 1, | ||||||||||||||||||||||||||||
| 5000, // 5 second timeout | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const result = files.length > 0 ? files[0].fsPath : undefined; | ||||||||||||||||||||||||||||
| this.binaryPathCache.set(cacheKey, result); | ||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||
| // Timeout or other error - log and cache undefined to avoid retrying | ||||||||||||||||||||||||||||
| // Note: Error logging is intentionally minimal to avoid noise in production | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just going to shut this PR down in favor of #17345, which works also works for my giant monorepo. |
||||||||||||||||||||||||||||
| // but helps with debugging when needed | ||||||||||||||||||||||||||||
| if (error instanceof Error && error.message === "Binary search timeout") { | ||||||||||||||||||||||||||||
| // Timeout is expected in large monorepos, no need to log | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| this.binaryPathCache.set(cacheKey, undefined); | ||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (!workspace.isTrusted) { | ||||||||||||||||||||||||||||
|
|
@@ -156,6 +333,15 @@ export class ConfigService implements IDisposable { | |||||||||||||||||||||||||||
| if (event.affectsConfiguration(ConfigService.namespace)) { | ||||||||||||||||||||||||||||
| this.vsCodeConfig.refresh(); | ||||||||||||||||||||||||||||
| isConfigChanged = true; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Clear cache when binary path settings change, as they affect binary resolution | ||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||
| event.affectsConfiguration(`${ConfigService.namespace}.path.oxlint`) || | ||||||||||||||||||||||||||||
| event.affectsConfiguration(`${ConfigService.namespace}.path.oxfmt`) || | ||||||||||||||||||||||||||||
| event.affectsConfiguration(`${ConfigService.namespace}.path.server`) | ||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||
| this.binaryPathCache.clear(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| for (const workspaceConfig of this.workspaceConfigs.values()) { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.