move spec linting to utility process#8853
Conversation
fb2ecb0 to
5ce5ab4
Compare
67efca6 to
791e3ef
Compare
| process.env['INSOMNIA_DATA_PATH'] || window.app.getPath('userData'), | ||
| `version-control/git/${gitRepositoryId}/other/.spectral.yaml`, | ||
| ); | ||
| const rulesetPath = gitRepositoryId |
There was a problem hiding this comment.
I suggest we keep process.env['INSOMNIA_DATA_PATH'] here
There was a problem hiding this comment.
I check it at runtime and its always undefined, but app.getPath('userData') is set in main.development at init time so its safe to remove here. It was previously used for inso code paths which don't touch actions and loaders
| // onmessage callback will be called several times in one promise, | ||
| // and promise can be resolved or rejected only once, so we cant reject it here | ||
| // NOTE: we will depend on the garbage collection to clean up the unresolved promises | ||
| const hasResultForThisTask = id === this.taskId && diagnostics; |
There was a problem hiding this comment.
We use a taskId to make sure we can use the latest lint result in the worker. So I think we also need this mechanism in the utility process.
There was a problem hiding this comment.
Good point it should have a race condition strategy, thanks. Although I'm not sure its possible for a short lived process to emit events from others running at the same time, I'll try to test it.
| ipcMainHandle('lintSpec', async (_, options: { documentContent: string; rulesetPath: string }) => { | ||
| const { documentContent, rulesetPath } = options; | ||
| return new Promise((resolve, reject) => { | ||
| const lintProcess = utilityProcess.fork(path.join(__dirname, 'main/lint-process.mjs')); |
There was a problem hiding this comment.
This function will run every time the user edits the YAML. We should share only one utility process and reuse it rather than create a new process every time. Too many processes may be a risk.
There was a problem hiding this comment.
We can try this: move the utilityProcess fork into a separate file.
// spectralUtilityProcess.ts
let spectralUtilityProcess;
const getSpectralUtilityProcess = () => {
if (!spectralUtilityProcess) {
spectralUtilityProcess = utilityProcess.fork(modulePath);
}
return spectralUtilityProcess;
};
There was a problem hiding this comment.
This is a good point, I'm leaning towards short lived processes over long running ones as its easier to reason about and I haven't read that short lived has any disadvantages yet. If you have some reading please share.
Also I'll discuss more with James as he's been working on a long running utility process.
There was a problem hiding this comment.
https://leapcell.io/blog/multi-threading-in-nodejs
Using fork(), each child process runs as a separate Node.js instance with its own V8 engine and event loop. This means that creating too many child processes may lead to high resource consumption.
The problem is not about short-lived or long-running processes; it is about the process count we create.
Spectral lint is sometimes not a very short task; if the file is very big, the lint process will spend more than 10 seconds, I remember.
If the user continues to input, multiple processes may be created at once in a short period of time, which may introduce performance or memory issues. I think using only 1 process or using worker threads is safer for us.
602a670 to
4fdc36e
Compare
gatzjames
left a comment
There was a problem hiding this comment.
Using the utility process for now seems the only reasonable way to move forward!
In the next pass we might want to changes how this works completely and connect it to the data changing.
One thing we figured out is that spectral can run in the browser but when imported in a worker it uses the node API and most probably the browser version relies on browser APIs being available.
If we find an alternative we can reconsider!
|
#7374 We tried running spectral in worker threads, just put here as a reference. |
* first pass * move lint worker * fix types * fix e2e test * check ruleset exists * move validation into util process * fix test again * mjs * make it clearer * bubble errors up in linter to a modal * fix e2e test * handle race condition
motivation: web worker sandbox requires nodeintegration be switched off. Spectral doesn't play nice with vite in browser esm mode so we need to run it in a seperate context. It's also quite heavy and slows down time to first paint so here we are running it in a utilityProcess which is like a child_process but electron native. The only unknown of this work is what happens if spectral hangs. Errors will be handled the same as before while editing but during actions will throw navigation exceptions
todo
I chose to use mjs in this instance since ts transpilation doesn't give us much benefit without HMR. This is worth revisiting if we decide to get hmr working with main as they do in electron-vite