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

LanguageClient.sendNotification fails due to change in state due to stopping client. #1616

Open
mmahrouss opened this issue Feb 18, 2025 · 1 comment
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@mmahrouss
Copy link

mmahrouss commented Feb 18, 2025

We are working on an LS for Odoo, and we have encountered an error that happens due to change in state.

Let me describe the scenario:

  • We have a listener on onDidChangeConfiguration that based on some condition closes the client with await client.stop(15000).
  • In the Client base code you also have a listener to sendNotification with the didChangeConfiguration Notification to be sent to the server.
  • Since we stop the client the state is updated, but it is updated after the check you have at the top of the function Here:
    public async sendNotification<P>(type: string | MessageSignature, params?: P): Promise<void> {
    if (this.$state === ClientState.StartFailed || this.$state === ClientState.Stopping || this.$state === ClientState.Stopped) {
    return Promise.reject(new ResponseError(ErrorCodes.ConnectionInactive, `Client is not running`));
    }
  • At that point the state is still running, so it goes forward until it calls $start here:
    // Ensure we have a connection before we force the document sync.
    const connection = await this.$start();
  • Then while it awaits this.start() which is probably already a resolved promise, the code is interrupted, and the stop function is called, so the state changes to stopping.
    private async $start(): Promise<Connection> {
    if (this.$state === ClientState.StartFailed) {
    throw new Error(`Previous start failed. Can't restart server.`);
    }
    await this.start();
    const connection = this.activeConnection();
    if (connection === undefined) {
    throw new Error(`Starting server failed`);
    }
    return connection;
    }
  • Then of course when it goes to this.activeConnection() it returns undefined, which then throws an error.
    private activeConnection(): Connection | undefined {
    return this.$state === ClientState.Running && this._connection !== undefined ? this._connection : undefined;
    }
  • However in this case from our POV, it is not an error, because it was trying to send a notification while the server is stopping, and the state was stopping/stopped. I think maybe there should be a guard to check the state before raising the error so that the state is up to date.
@dbaeumer dbaeumer added this to the Backlog milestone Feb 20, 2025
@dbaeumer
Copy link
Member

Thanks for the analysis. Needs some thinking...

@dbaeumer dbaeumer added the bug Issue identified by VS Code Team member as probable bug label Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

2 participants