Add timeouts for every process (except the ones that run in server mode) spawned by the extension

[ramya-rao-a]

This extension depends on a host Go tools to provide various features.

As seen in #2084, any of these tools can get into a non responsive state or run for way too long. To avoid bad user experience we should apply a timeout on such scenarios and kill the processes on timeout

[lggomez]

The execFile method accepts a timeout parameter in milliseconds, time after which the child process will be sent a SIGTERM signal

This fallback is really needed, as guru is still unusable on my workspace since it kills my mac with a couple dozen processes, but which is an acceptable default value? also, it would need to be either:

• A single configuration, but with a sensible default so it doesn't end prematurely 'long' running processes like static analyzers
• A configuration per file, being able to set up different values according to each tool

... except the ones that run in server mode ...

I guess you're talking about the debugger and the language server. Is there any other case I'm missing?

[ramya-rao-a]

I guess you're talking about the debugger and the language server. Is there any other case I'm missing?

gocode is initially run in server mode. Subsequent completion requests make client type requests.

which is an acceptable default value

There are 3 types of operations

• "onSave" operations like build, lint, vet
• "onType" operations like completions and live-errors.
• All other operations that are triggered by a command.

The first 2 types should have shorter timeouts, and the 3rd one can afford to have longer timeout.

VS Code has a timeout for the formatOnSave feature (editor.formatOnSaveTimeout)

For us, I can think of a single setting say go.operationTimeout which can take an object like

{
   "buildOnSave": 1000
    "buildOnCommand": 2000
    "completion": 2000
    ....
}

[lggomez]

VS Code has a timeout for the formatOnSave feature (editor.formatOnSaveTimeout)

This timeout should send a CancellationToken to the extension so there isn't anything to do on our side right?

For us, I can think of a single setting say go.operationTimeout which can take an object like

 {
    "buildOnSave": 1000
     "buildOnCommand": 2000
     "completion": 2000
     ....
 }

I was thinking on implementing an execFile wrapper in utils that handles the configuration retrieval and timeout parameter according to an enum parameter with these values

[ramya-rao-a]

This timeout should send a CancellationToken to the extension so there isn't anything to do on our side right?

Yes.

I was thinking on implementing an execFile wrapper in utils that handles the configuration retrieval and timeout parameter according to an enum parameter with these values

You would have to maintain a mapping between the enum value and the name used in the configuration.

Most of our files already access the configuration object, so doing the configuration retrieval in the the wrapper doesnt give much benefits.

[lggomez]

So, these are all the execFile calls we currently do (excluding the debug adapter):

goModifytags.ts:130:	let p = cp.execFile(gomodifytags, args, { env: getToolsEnvVars() }, (err, stdout, stderr) => {
goGetPackage.ts:27:	cp.execFile(goRuntimePath, ['get', '-v', importPath], { env }, (err, stdout, stderr) => {
goSymbol.ts:111:		p = cp.execFile(gosyms, args, { maxBuffer: 1024 * 1024, env }, (err, stdout, stderr) => {
goSymbol.ts:130:		cp.execFile(goExecutable, ['env', 'GOROOT'], (err, stdout) => {
goModules.ts:15:		cp.execFile(goExecutable, ['env', 'GOMOD'], { cwd: folderPath, env: getToolsEnvVars() }, (err, stdout) => {
goInstallTools.ts:326:						cp.execFile(gometalinterBinPath, ['--install'], { env: envForTools }, (err, stdout, stderr) => {
goInstallTools.ts:345:					cp.execFile(toolBinPath, ['close'], {}, (err, stdout, stderr) => {
goInstallTools.ts:365:				cp.execFile(goRuntimePath, args, { env: envForTools }, (err, stdout, stderr) => {
goInstallTools.ts:368:						cp.execFile(goRuntimePath, args, { env: envForTools }, callback);
goInstallTools.ts:371:						cp.execFile(goRuntimePath, ['build', '-o', outputFile, getToolImportPath(tool, goVersion)], { env: envForTools }, callback);
goInstallTools.ts:414:		cp.execFile(goRuntimePath, ['env', 'GOPATH', 'GOROOT'], (err, stdout, stderr) => {
goPlayground.ts:44:		const p = execFile(binaryLocation, [...cliArgs, '-'], (err, stdout, stderr) => {
goImport.ts:147:	cp.execFile(goRuntimePath, ['list', '-f', '{{.Dir}}', importPath], { env }, (err, stdout, stderr) => {
goFillStruct.ts:61:		let p = cp.execFile(fillstruct, args, { env: getToolsEnvVars() }, (err, stdout, stderr) => {
goBrowsePackage.ts:58:	cp.execFile(goRuntimePath, ['list', '-f', '{{.Dir}}:{{.GoFiles}}:{{.TestGoFiles}}:{{.XTestGoFiles}}', pkg], options, (err, stdout, stderr) => {
util.ts:243:		cp.execFile(goRuntimePath, ['version'], {}, (err, stdout, stderr) => {
util.ts:645:		p = cp.execFile(cmd, args, { env: env, cwd: cwd }, (err, stdout, stderr) => {
util.ts:924:			const p = cp.execFile(goRuntimePath, args, { env, cwd }, (err, stdout, stderr) => {
goRename.ts:46:			p = cp.execFile(gorename, gorenameArgs, {env}, (err, stdout, stderr) => {
goInstall.ts:44:	cp.execFile(goRuntimePath, args, { env, cwd }, (err, stdout, stderr) => {
goTypeDefinition.ts:57:			let process = cp.execFile(goGuru, args, { env }, (err, stdout, stderr) => {
goReferences.ts:42:			let process = cp.execFile(goGuru, args, { env }, (err, stdout, stderr) => {
goSuggest.ts:419:			cp.execFile(gocode, ['set'], { env }, (err, stdout, stderr) => {
goSuggest.ts:436:					cp.execFile(gocode, ['set', name, value], { env }, (err, stdout, stderr) => {
goDeclaration.ts:117:		p = cp.execFile(godefPath, args, { env, cwd: input.cwd }, (err, stdout, stderr) => {
goDeclaration.ts:197:		p = cp.execFile(gogetdoc, gogetdocFlags, { env, cwd: input.cwd }, (err, stdout, stderr) => {
goDeclaration.ts:258:		p = cp.execFile(guru, ['-json', '-modified', 'definition', input.document.fileName + ':#' + offset.toString()], { env }, (err, stdout, stderr) => {
goGenerateTests.ts:152:		cp.execFile(cmd, args, { env: getToolsEnvVars() }, (err, stdout, stderr) => {
goLiveErrors.ts:66:	let p = cp.execFile(gotypeLive, args, { env }, (err, stdout, stderr) => {
goImpl.ts:42:	let p = cp.execFile(goimpl, args, { env: getToolsEnvVars(), cwd: dirname(vscode.window.activeTextEditor.document.fileName) }, (err, stdout, stderr) => {
goImplementations.ts:45:			let listProcess = cp.execFile(getBinPath('go'), ['list', '-e', '-json'], { cwd: root, env }, (err, stdout, stderr) => {
goImplementations.ts:61:				let guruProcess = cp.execFile(goGuru, args, { env }, (err, stdout, stderr) => {
goOutline.ts:92:		p = cp.execFile(gooutline, gooutlineFlags, { env: getToolsEnvVars() }, (err, stdout, stderr) => {

If we had to classify them all according to use case but at the same time differentiating against execution times we would have something like this:

{
	"go.operationTimeout": {
		"buildOnSave": 2000, //go build
		"buildOnCommand": 2000, //go build
		"runtimeDefault": 2000, //go xxx (goRuntimePath)
		"executeOnSave": 2000,
		"executeOnCommand": 2000,
		"executeOnType": 2000 //completions, live-errors
	}
}

This may be a bit verbose for a configuration though, so tell me what you think

[ramya-rao-a]

The verbosity is fine

• I would replace "execute" with "operation" in executeOnSave, executeOnCommand and executeOnType
• runTimeDefault will need some more thought. We use this for go version, go env, go list, go doc, go vet, go build, go test, go get, go install. They all cannot fall under the same bucket and use the same timeout.

[lggomez]

In terms of plausible execution times the tools version, env, list and doc might appear to be in a lower tier; build, test, get and install can be long running in comparison with the former

…

On Wed, Mar 6, 2019, 03:39 Ramya Rao ***@***.***> wrote: The verbosity is fine - I would replace "execute" with "operation" in executeOnSave, executeOnCommand and executeOnType - runTimeDefault will need some more thought. We use this for go version, go env, go list, go doc, go vet, go build, go test, go get, go install. They all cannot fall under the same bucket and use the same timeout. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGCH_zrQSWNf96xKisQdtmj_l8uR6rYUks5vT2KFgaJpZM4bOS0a> .

[ramya-rao-a]

Some operations that use go list may run longer. Since the results are cached, it would be ok for it to run that way.

[lggomez]

Then we can move it to the group of long running tasks. Results may be cached but any initial run will do it in full extent, so I prefer to be lax rather than conservative about these thresholds. I usually find myself doing a lot of GOCACHE=off calls to go tools because from time to time I have to switch across go versions in our projects (I use a fork of gvm for that matter)

[ramya-rao-a]

Closing in favor of golang/vscode-go#91 now that we are moving repos. The PR will be the place of further discussion here.

See We are moving section in our readme for more details.

Thanks for all your support @lggomez!