Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid race condition on worker disposal #485

Merged

Conversation

morgilad
Copy link
Collaborator

@morgilad morgilad commented Oct 7, 2024

Disposing of Kusto Worker on setLanguageSettings Call

Every time setLanguageSettings is called, the old Kusto worker is disposed of, and a new one is created. This means that if the old worker is in the middle of handling requests, those requests will be terminated before completion.

What This Causes

We register the semantic token provider after worker.setSchema has completed. Since setLanguageSettings is called when entering the query page editor, the worker gets disposed of during that call, leaving the setSchema promise unresolved. As a result, the semantic tokens provider is not registered.

The Workaround Solution

The solution is a bit of a workaround—apologies for that. I wait for 5 seconds before disposing of the old worker to allow it to finish processing previous requests. Before that, I reset the workerManager private member of workerDetails, so new requests will create a new worker and be applied to the new instance.

Is Disposing the Old Worker Necessary?

I did consider whether disposing of the old worker when setLanguageSettings is called is truly necessary. I believe it isn’t. However, Noam and I decided not to remove this behavior currently because it might impact unknown scenarios that we cannot easily test. Therefore, this change is not suitable when a Quick Fix Engineering (QFE) is required.

@morgilad morgilad requested a review from maxburs October 7, 2024 16:49
@@ -3,41 +3,50 @@ import type * as monaco from 'monaco-editor/esm/vs/editor/editor.api';
import type { LanguageServiceDefaults } from './monaco.contribution';
import type { IKustoWorkerImpl } from './kustoWorker';

interface WorkerDetails {
_worker: monaco.editor.MonacoWebWorker<IKustoWorkerImpl>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove the prefix _

@morgilad
Copy link
Collaborator Author

morgilad commented Oct 7, 2024

Merging this in order to start publishing the new version as it takes a few hours.
If Ill get important comments I will postpone updating the version in the main project.

@morgilad morgilad merged commit 1f40b10 into master Oct 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants