Skip to content
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

watcher - stop using proposed watcher API for TS #228703

Merged
merged 6 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion extensions/typescript-language-features/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"aiKey": "0c6ae279ed8443289764825290e4f9e2-1a736e7c-1324-4338-be46-fc2a58ae4d14-7255",
"enabledApiProposals": [
"workspaceTrust",
"createFileSystemWatcher",
"multiDocumentHighlightProvider",
"mappedEditsProvider",
"codeActionAI",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export interface TypeScriptServiceConfiguration {
readonly enableProjectDiagnostics: boolean;
readonly maxTsServerMemory: number;
readonly enablePromptUseWorkspaceTsdk: boolean;
readonly useVsCodeWatcher: boolean; // TODO@bpasero remove this setting eventually
readonly useVsCodeWatcher: boolean;
readonly watchOptions: Proto.WatchOptions | undefined;
readonly includePackageJsonAutoImports: 'auto' | 'on' | 'off' | undefined;
readonly enableTsServerTracing: boolean;
Expand Down Expand Up @@ -223,7 +223,12 @@ export abstract class BaseServiceConfigurationProvider implements ServiceConfigu
}

private readUseVsCodeWatcher(configuration: vscode.WorkspaceConfiguration): boolean {
return configuration.get<boolean>('typescript.tsserver.experimental.useVsCodeWatcher', false);
const watcherExcludes = configuration.get<Record<string, boolean>>('files.watcherExclude') ?? {};
if (watcherExcludes['**/node_modules/*/**'] /* VS Code default prior to 1.94.x */ === true) {
return false; // we cannot use the VS Code watcher if node_modules are excluded
}

return configuration.get<boolean>('typescript.tsserver.experimental.useVsCodeWatcher', true);
}

private readWatchOptions(configuration: vscode.WorkspaceConfiguration): Proto.WatchOptions | undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,7 @@ export default class TypeScriptServiceClient extends Disposable implements IType
ignoreChangeEvents?: boolean,
) {
const disposable = new DisposableStore();
const watcher = disposable.add(vscode.workspace.createFileSystemWatcher(pattern, { excludes: [] /* TODO:: need to fill in excludes list */, ignoreChangeEvents }));
const watcher = disposable.add(vscode.workspace.createFileSystemWatcher(pattern, undefined, ignoreChangeEvents));
disposable.add(watcher.onDidChange(changeFile =>
this.addWatchEvent(id, 'updated', changeFile.fsPath)
));
Expand Down
1 change: 0 additions & 1 deletion extensions/typescript-language-features/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"include": [
"src/**/*",
"../../src/vscode-dts/vscode.d.ts",
"../../src/vscode-dts/vscode.proposed.createFileSystemWatcher.d.ts",
"../../src/vscode-dts/vscode.proposed.codeActionAI.d.ts",
"../../src/vscode-dts/vscode.proposed.codeActionRanges.d.ts",
"../../src/vscode-dts/vscode.proposed.mappedEditsProvider.d.ts",
Expand Down
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions remote/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

116 changes: 60 additions & 56 deletions src/vs/platform/files/node/watcher/baseWatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ import { Emitter, Event } from '../../../../base/common/event.js';
import { FileChangeType, IFileChange } from '../../common/files.js';
import { URI } from '../../../../base/common/uri.js';
import { DeferredPromise, ThrottledDelayer } from '../../../../base/common/async.js';
import { hash } from '../../../../base/common/hash.js';

interface ISuspendedWatchRequest {
readonly id: number;
readonly correlationId: number | undefined;
readonly path: string;
}

export abstract class BaseWatcher extends Disposable implements IWatcher {

Expand All @@ -22,11 +29,11 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
protected readonly _onDidWatchFail = this._register(new Emitter<IUniversalWatchRequest>());
private readonly onDidWatchFail = this._onDidWatchFail.event;

private readonly allNonCorrelatedWatchRequests = new Set<IUniversalWatchRequest>();
private readonly allCorrelatedWatchRequests = new Map<number /* correlation ID */, IWatchRequestWithCorrelation>();
private readonly correlatedWatchRequests = new Map<number /* request ID */, IWatchRequestWithCorrelation>();
private readonly nonCorrelatedWatchRequests = new Map<number /* request ID */, IUniversalWatchRequest>();

private readonly suspendedWatchRequests = this._register(new DisposableMap<number /* correlation ID */>());
private readonly suspendedWatchRequestsWithPolling = new Set<number /* correlation ID */>();
private readonly suspendedWatchRequests = this._register(new DisposableMap<number /* request ID */>());
private readonly suspendedWatchRequestsWithPolling = new Set<number /* request ID */>();

private readonly updateWatchersDelayer = this._register(new ThrottledDelayer<void>(this.getUpdateWatchersDelay()));

Expand All @@ -37,56 +44,52 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
constructor() {
super();

this._register(this.onDidWatchFail(request => this.handleDidWatchFail(request)));
}

private handleDidWatchFail(request: IUniversalWatchRequest): void {
if (!this.isCorrelated(request)) {

// For now, limit failed watch monitoring to requests with a correlationId
// to experiment with this feature in a controlled way. Monitoring requests
// requires us to install polling watchers (via `fs.watchFile()`) and thus
// should be used sparingly.
//
// TODO@bpasero revisit this in the future to have a more general approach
// for suspend/resume and drop the `legacyMonitorRequest` in parcel.
// One issue is that we need to be able to uniquely identify a request and
// without correlation that is actually harder...

return;
}

this.suspendWatchRequest(request);
this._register(this.onDidWatchFail(request => this.suspendWatchRequest({
id: this.computeId(request),
correlationId: this.isCorrelated(request) ? request.correlationId : undefined,
path: request.path
})));
}

protected isCorrelated(request: IUniversalWatchRequest): request is IWatchRequestWithCorrelation {
return isWatchRequestWithCorrelation(request);
}

private computeId(request: IUniversalWatchRequest): number {
if (this.isCorrelated(request)) {
return request.correlationId;
} else {
// Requests without correlation do not carry any unique identifier, so we have to
// come up with one based on the options of the request. This matches what the
// file service does (vs/platform/files/common/fileService.ts#L1178).
return hash(request);
}
}

async watch(requests: IUniversalWatchRequest[]): Promise<void> {
if (!this.joinWatch.isSettled) {
this.joinWatch.complete();
}
this.joinWatch = new DeferredPromise<void>();

try {
this.allCorrelatedWatchRequests.clear();
this.allNonCorrelatedWatchRequests.clear();
this.correlatedWatchRequests.clear();
this.nonCorrelatedWatchRequests.clear();

// Figure out correlated vs. non-correlated requests
for (const request of requests) {
if (this.isCorrelated(request)) {
this.allCorrelatedWatchRequests.set(request.correlationId, request);
this.correlatedWatchRequests.set(request.correlationId, request);
} else {
this.allNonCorrelatedWatchRequests.add(request);
this.nonCorrelatedWatchRequests.set(this.computeId(request), request);
}
}

// Remove all suspended correlated watch requests that are no longer watched
for (const [correlationId] of this.suspendedWatchRequests) {
if (!this.allCorrelatedWatchRequests.has(correlationId)) {
this.suspendedWatchRequests.deleteAndDispose(correlationId);
this.suspendedWatchRequestsWithPolling.delete(correlationId);
// Remove all suspended watch requests that are no longer watched
for (const [id] of this.suspendedWatchRequests) {
if (!this.nonCorrelatedWatchRequests.has(id) && !this.correlatedWatchRequests.has(id)) {
this.suspendedWatchRequests.deleteAndDispose(id);
this.suspendedWatchRequestsWithPolling.delete(id);
}
}

Expand All @@ -97,31 +100,32 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
}

private updateWatchers(delayed: boolean): Promise<void> {
return this.updateWatchersDelayer.trigger(() => this.doWatch([
...this.allNonCorrelatedWatchRequests,
...Array.from(this.allCorrelatedWatchRequests.values()).filter(request => !this.suspendedWatchRequests.has(request.correlationId))
]), delayed ? this.getUpdateWatchersDelay() : 0);
const nonSuspendedRequests: IUniversalWatchRequest[] = [];
for (const [id, request] of [...this.nonCorrelatedWatchRequests, ...this.correlatedWatchRequests]) {
if (!this.suspendedWatchRequests.has(id)) {
nonSuspendedRequests.push(request);
}
}

return this.updateWatchersDelayer.trigger(() => this.doWatch(nonSuspendedRequests), delayed ? this.getUpdateWatchersDelay() : 0);
}

protected getUpdateWatchersDelay(): number {
return 800;
}

isSuspended(request: IUniversalWatchRequest): 'polling' | boolean {
if (typeof request.correlationId !== 'number') {
return false;
}

return this.suspendedWatchRequestsWithPolling.has(request.correlationId) ? 'polling' : this.suspendedWatchRequests.has(request.correlationId);
const id = this.computeId(request);
return this.suspendedWatchRequestsWithPolling.has(id) ? 'polling' : this.suspendedWatchRequests.has(id);
}

private async suspendWatchRequest(request: IWatchRequestWithCorrelation): Promise<void> {
if (this.suspendedWatchRequests.has(request.correlationId)) {
private async suspendWatchRequest(request: ISuspendedWatchRequest): Promise<void> {
if (this.suspendedWatchRequests.has(request.id)) {
return; // already suspended
}

const disposables = new DisposableStore();
this.suspendedWatchRequests.set(request.correlationId, disposables);
this.suspendedWatchRequests.set(request.id, disposables);

// It is possible that a watch request fails right during watch()
// phase while other requests succeed. To increase the chance of
Expand All @@ -139,24 +143,24 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
this.updateWatchers(true /* delay this call as we might accumulate many failing watch requests on startup */);
}

private resumeWatchRequest(request: IWatchRequestWithCorrelation): void {
this.suspendedWatchRequests.deleteAndDispose(request.correlationId);
this.suspendedWatchRequestsWithPolling.delete(request.correlationId);
private resumeWatchRequest(request: ISuspendedWatchRequest): void {
this.suspendedWatchRequests.deleteAndDispose(request.id);
this.suspendedWatchRequestsWithPolling.delete(request.id);

this.updateWatchers(false);
}

private monitorSuspendedWatchRequest(request: IWatchRequestWithCorrelation, disposables: DisposableStore): void {
private monitorSuspendedWatchRequest(request: ISuspendedWatchRequest, disposables: DisposableStore): void {
if (this.doMonitorWithExistingWatcher(request, disposables)) {
this.trace(`reusing an existing recursive watcher to monitor ${request.path}`);
this.suspendedWatchRequestsWithPolling.delete(request.correlationId);
this.suspendedWatchRequestsWithPolling.delete(request.id);
} else {
this.doMonitorWithNodeJS(request, disposables);
this.suspendedWatchRequestsWithPolling.add(request.correlationId);
this.suspendedWatchRequestsWithPolling.add(request.id);
}
}

private doMonitorWithExistingWatcher(request: IWatchRequestWithCorrelation, disposables: DisposableStore): boolean {
private doMonitorWithExistingWatcher(request: ISuspendedWatchRequest, disposables: DisposableStore): boolean {
const subscription = this.recursiveWatcher?.subscribe(request.path, (error, change) => {
if (disposables.isDisposed) {
return; // return early if already disposed
Expand All @@ -178,7 +182,7 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
return false;
}

private doMonitorWithNodeJS(request: IWatchRequestWithCorrelation, disposables: DisposableStore): void {
private doMonitorWithNodeJS(request: ISuspendedWatchRequest, disposables: DisposableStore): void {
let pathNotFound = false;

const watchFileCallback: (curr: Stats, prev: Stats) => void = (curr, prev) => {
Expand Down Expand Up @@ -215,7 +219,7 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
}));
}

private onMonitoredPathAdded(request: IWatchRequestWithCorrelation) {
private onMonitoredPathAdded(request: ISuspendedWatchRequest): void {
this.trace(`detected ${request.path} exists again, resuming watcher (correlationId: ${request.correlationId})`);

// Emit as event
Expand All @@ -236,14 +240,14 @@ export abstract class BaseWatcher extends Disposable implements IWatcher {
this.suspendedWatchRequestsWithPolling.clear();
}

protected traceEvent(event: IFileChange, request: IUniversalWatchRequest): void {
protected traceEvent(event: IFileChange, request: IUniversalWatchRequest | ISuspendedWatchRequest): void {
if (this.verboseLogging) {
const traceMsg = ` >> normalized ${event.type === FileChangeType.ADDED ? '[ADDED]' : event.type === FileChangeType.DELETED ? '[DELETED]' : '[CHANGED]'} ${event.resource.fsPath}`;
this.traceWithCorrelation(traceMsg, request);
}
}

protected traceWithCorrelation(message: string, request: IUniversalWatchRequest): void {
protected traceWithCorrelation(message: string, request: IUniversalWatchRequest | ISuspendedWatchRequest): void {
if (this.verboseLogging) {
this.trace(`${message}${typeof request.correlationId === 'number' ? ` <${request.correlationId}> ` : ``}`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class NodeJSFileWatcherLibrary extends Disposable {

private readonly excludes = parseWatcherPatterns(this.request.path, this.request.excludes);
private readonly includes = this.request.includes ? parseWatcherPatterns(this.request.path, this.request.includes) : undefined;
private readonly filter = isWatchRequestWithCorrelation(this.request) ? this.request.filter : undefined; // TODO@bpasero filtering for now is only enabled when correlating because watchers are otherwise potentially reused
private readonly filter = isWatchRequestWithCorrelation(this.request) ? this.request.filter : undefined; // filtering is only enabled when correlating because watchers are otherwise potentially reused

private readonly cts = new CancellationTokenSource();

Expand Down
Loading
Loading