While debugging why the server didn't seem to always shut down cleanly, I noticed a few issues that may be factors:
1. We kill the analyzer 100ms after deactivate() is called regardless
This code was apparently an attempt to fix #5084, but 100ms seems quite short given other timeouts (for example the server has a 250ms timeout on sending the shutdown analytics, but it also runs other code during shutdown).
| this.disposables.push({ dispose: () => { setTimeout(() => process.kill(), 100); } }); |
2. We never block deactive() for graceful shutdowns
In deactivate, we call analyzer.dispose(), which is implemented like this:
| public dispose(): void | Promise<void> { | |
| disposeAll(this.disposables); | |
| } |
We do not await the result of disposeAll (and currently, it calls void d.dispose() for each item in disposables), which means we will start disposing things but not actually block deactivate on them, so unless something else holds up the extension host shutting down, the extension host might get terminated (and might take child processes with it).
Fixes
Probably we should:
- Remove and
voids on calling dispose methods, and instead always await all disposes everywhere, including ondisposeAll - Add a timeout for calls to
disposeat top-levels (for example indeactivate, maybe in Restart Analysis Server command, etc.) - Prefer to dispose things in parallel where possible, so one thing being slow/hanging doesn't prevent subsequent objects from being disposed (we need to be careful of dependencies)