Skip to content

Extension restart fix #433

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shivam71
Copy link
Member

@shivam71 shivam71 commented Jun 17, 2025

Issue
Post activating the extension one of the two cases happen

1)JDK Found
2) No JDK Found
In case of "No JDK Found" we enter a bad state where even if the user changes the jdk path to some valid path we cannot restart unless the user reloads .

In case initially JDK found but later user changes the path to an invalid jdk home then also we enter No JDK Found state.
Fix
Moved the reload window pop up from jdk downloader to configuration change listener(restartExtension) triggered whenever any configuration is changed and extension is in bad state.
This gives user a prompt to reload window so that extension can come out of the bad state.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 17, 2025
@shivam71 shivam71 force-pushed the extension-restart-fix-final branch 2 times, most recently from 6ea48bc to d3a16fe Compare June 25, 2025 09:47
Moved the reload window pop up from jdk downloader to configuration change listener(restartExtension) triggered  whenever any configuration is changed and extension is in bad state.
@shivam71 shivam71 force-pushed the extension-restart-fix-final branch from d3a16fe to d3721f8 Compare July 3, 2025 08:29
Copy link
Member

@Achal1607 Achal1607 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

Copy link
Member

@sid-srini sid-srini left a comment

Choose a reason for hiding this comment

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

Please rebase the branch to main in order to remove unrelated changes in l10n files. Thanks.

--- Update: Sorry, please ignore. I mistook the highlight due to the added , before the necessary changes.

@shivam71
Copy link
Member Author

While checking for the consistency I find that the deactivate function in extension.ts killing the process first

export function deactivate(): Thenable<void> {
    Telemetry.enqueueCloseEvent();
    const process = globalState.getNbProcessManager()?.getProcess();
    if (process != null) {
        process?.kill();
    }
    return globalState.getClientPromise().stopClient();
}

This function is called by vscode only (when closing window) and is for cleaning up . So we can do the following

export function deactivate(): Thenable<void> {
    Telemetry.enqueueCloseEvent();
    const clientStopped = globalState.getClientPromise().stopClient();
    const process = globalState.getNbProcessManager()?.getProcess();
    if (process != null) {
        process?.kill();
    }
    return clientStopped;
}

Note

@shivam71
Copy link
Member Author

As per the LSP protocol the client manages the server life cycle .

So as per the protocol the client sends a Shutdown Request to the server indicating the server to prepare for shutdown . The sever must respond with errors if any that happen during the shutdown .

On receiving the Shutdown request's response from the server the client sends an Exit notification which when the server receives must kill itself i.e exit the process .

client.stop() (vscode language client implementation) does exactly this but our netbeans lsp server doesn't implement the exit method so it keeps on running post the exit notification . Hence we have to do the mannual killing afterwards.

Implementing the exit method in the netbeans lsp server will help us avoid this manual killing altogether and make the code easier to understand as well as adhere with the lsp protocol spec.

@Achal1607 Achal1607 added this to the JVSC 24.1.1 milestone Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants