TerminalShellExecution#createDataStream

[jrieken]

Testing #208462

The TerminalShellExecution#createDataStream should be a plain property and not a method

[Tyriar]

I was thinking about this guideline when I switched it to a method as it's creating a new object every call:

Note: Providing synchronous data comes with some additional engineering cost and will consume extra memory. However, it guarantees fast (zero-cost) data reading.

Can switch it back, is it worth a discussion in the API sync?

[jrieken]

vscode/src/vs/workbench/api/common/extHostTerminalShellIntegration.ts

Lines 243 to 258 in b0d975f

	createDataStream(): AsyncIterable<string> {
	return that._createDataStream();
	}
	};
	}
	private _createDataStream(): AsyncIterable<string> {
	if (!this._dataStream) {
	if (this._exitCodeResolve === undefined) {
	return AsyncIterableObject.EMPTY;
	}
	this._dataStream = new ShellExecutionDataStream();
	}
	return this._dataStream.createIterable();
	}

I don't think it does create a new object but that's also not the point here. Your data is never sync with an async iterable but the point is the consumer should just have TerminalShellExecution#stream which is still an async iterable (and likely implemented as lazy getter)

[Tyriar]

@jrieken I might be doing something wrong as I don't think I've used an async iterable before, but doesn't this need to return a separate iterable per call in order to support multiple extensions? Currently the ShellExecutionDataStream is a multiplexer for multiple iterators:

vscode/src/vs/workbench/api/common/extHostTerminalShellIntegration.ts

Line 256 in b0d975f

return this._dataStream.createIterable();

vscode/src/vs/workbench/api/common/extHostTerminalShellIntegration.ts

Lines 278 to 285 in b0d975f

	createIterable(): AsyncIterable<string> {
	const barrier = this._barrier = new Barrier();
	const iterable = new AsyncIterableObject<string>(async emitter => {
	this._emitters.push(emitter);
	await barrier.wait();
	});
	return iterable;
	}

[jrieken]

Oh, I missed the createIterable invocation inside createDataStream.

[jrieken]

Maybe a better name for a method would be read or readData.