Review places where we use IDisposable[] in the code

[mjbvz]

Problem

Our code commonly uses IDisposable[] to track multiple disposables. This is not safe and may leak disposables. Consider:

const disposables: IDisposable[] = [];
disposables.push(new DisposableThing());
dispose(disposables);
disposables.push(new DisposableThing2());

This results in DisposableThing2 being leaked. This sequence of calls can easily happen in async code or in complex sync code

Proposed Fix

Review places where we use IDisposable[]. In almost all cases, we should either:

• For classes, extend Disposable instead of creating a disposables private property. The Disposable class correctly handles the above case.

• In place of a IDisposable[], use DisposableStore which was added by Add DisposableStore for use in VS Code #74242 for this very reason.

[Tyriar]

@mjbvz on some objects I need to track isDisposed in order to ensure things don't happen after disposal, should that be exposed on Disposable too? An example:

vscode/src/vs/workbench/contrib/terminal/node/terminalProcess.ts

Lines 199 to 211 in 8a6ddac

	public resize(cols: number, rows: number): void {
	if (this._isDisposed) {
	return;
	}
	// Ensure that cols and rows are always >= 1, this prevents a native
	// exception in winpty.
	if (this._ptyProcess) {
	cols = Math.max(cols, 1);
	rows = Math.max(rows, 1);
	this._logService.trace('IPty#resize', cols, rows);
	this._ptyProcess.resize(cols, rows);
	}
	}

This would either throw an exception (maybe native and crash) if the process has been disposed.

[mjbvz]

Yes I'm ok to exposing this on Disposable. There are a few places in the code were we re-implement this same solution

[jrieken]

🛑 Can we please make this a team effort instead of @mjbvz making all changes across ownership. Apart from the merge conflict potential and introducing bugs, there is a lot of personal taste in this. A change like this really makes me sad: 6b7b5c0#diff-2754a48a0975eb32f8b8ff03d9d67478R16.

I generally favour composition over inheritance, esp. when it comes to Disposable. For me this._register(something) doesn't explain itself as nicely as this._disposables.add(something) and for that reason my code usually doesn't use 'extends Disposable'. Again, this is personal, like _ for private fields, like omitting the public-modifier, or like undefined vs null, but for that exact reason I don't want foreign looking changes. To make matters worst everything came in as monster commits that cannot be undone.

Don't get me wrong, I am not against the "stricter disposable"-effort, even though I haven't seem much evidence or leaks because of register-after-dispose cases. What I am against is mass changes across ownership boundaries without notice - we don't accept PRs that do this and we should respect that rule ourself.

[jrieken]

cc @microsoft/vscode

[joaomoreno]

I generally favour composition over inheritance

+1000. That extends Disposable just feels terribly wrong to me. It also just forces one thing: that a class can't dispose its children until it itself is disposed. I have complex use cases for IDisposable collections which don't conform to this pattern.

This results in DisposableThing2 being leaked. This sequence of calls can easily happen in async code or in complex sync code

Leaks like this happen due to poor code reasoning, they are not inherent to this code pattern, imo.

[mjbvz]

@jrieken I apologize if it felt like I was stepping on your toes here. For one, the size of the push to master got away from me. These changes should have gone in more incrementally.

Let me provide some additional context on why I think this work is important and why the changes were made in the way there were made.

---

I know that I've written incorrect code that uses IDisposable[] incorrectly, and I know with a near certainty that I will write incorrect code like this again in the future. It doesn't even have to be new code, maybe while refactoring an unrelated area I'll break something because the previous implicit assumptions were no longer true.

Changing our internal APIs to make unsafe patterns like this more difficult to write is the right approach in my opinion, even if we are not aware of incorrect usage causing any issues at the moment. That's why I have been looking into disposables for debt weak and that's where the introduction of a DisposableStore comes in. But in practice this work requires touching a lot of files in our codebase.

While adding DisposableStore, I also found that a large number of classes were re-implementing the same disposable management logic over and over again:

class Foo {
    private _disposables: IDisposable[] = [];

    ...

    dispose() {
        this._disposables = dispose(_disposables);
    }
}

The repeated code is what made refactoring to use a DisposableStore very tedious. I wanted to make sure that if we ever wanted to make a similar change to our disposable logic, that we would not run into this same issue again. That means centralizing this pattern and making it consistent across the codebase. This is where inheriting from Disposable comes in. One side benefit of moving most disposable management logic to a few points in the code (such as DisposableStore and Disposable) is that we can do thing like start writing basic runtime logic to track leaked disposable. I've use that to catch a number of real cases where we not disposing of objects properly

I don't think inheritance is a great solution, but it's the only approach that I've found so far that works well with TS. If anyone has another idea on how on how we can better share code for disposable objects, let me know

---

Ultimately I think we should work towards standardizing on common, safe patterns across all of our codebase. What these patterns should be is something we should discuss together. In this case, I did not alert enough team members to the disposable work I was interested in, or provide enough time for feedback or discussion on the proposed changes. I will try to do better at this in the future

[jrieken]

Bottom line is that everyone agrees that DisposableStore and Disposable have a clear advantage over the existing IDisposable[] pattern. People have personal preferences and can freely decide which of the two to use.

[jrieken]

I have pushed a change that makes events accept a disposable store (in addition to IDisposable[], which is API (!)), e.g. this._onDidChange(this._handler, this, this._disposableStore)

[mjbvz]

Closing as I don't think we can/want to replace all instances. Almost all new code should be using DisposableStore though as prevents many common types of bugs around disposables