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.

Sign up for GitHub

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

Jump to bottom

LSP client id is out of bounds #39548

Closed
Rubonnek opened this issue Jun 14, 2020 · 5 comments · Fixed by #39556
Closed

LSP client id is out of bounds #39548

Rubonnek opened this issue Jun 14, 2020 · 5 comments · Fixed by #39556
Assignees
@akien-mga
Labels
bug regression topic:gdscript
Milestone
4.0

Comments

@Rubonnek
Copy link
Member

Godot version:
6efab27

OS/device including version:
Arch Linux

Issue description:
Connecting and disconnecting LSP clients stops the LSP server from working properly.

Steps to reproduce:

  1. Connect 1 client to the LSP server
  2. Disconnect the client
  3. Attempt to connect the client again -- you'll get:
 modules/gdscript/language_server/gdscript_language_protocol.cpp:283 - Index (uint64_t)p_client_id = 1 is out of bounds (clients.size() = 1).
@Rubonnek
Copy link
Member Author

This issue was introduced on 3edae03

@akien-mga
Copy link
Member

The error message was added by 3edae03, but if you revert it, the above steps should give you a crash instead of an error.

@akien-mga akien-mga added this to the 4.0 milestone Jun 15, 2020
@akien-mga
Copy link
Member

CC @Geequlim @ofrank123 @NathanLovato

@Rubonnek
Copy link
Member Author

Rubonnek commented Jun 15, 2020 •
edited
Loading

@akien-mga I don't get a crash with vim-lsp after reverting the commit from the 3.2 branch under Arch Linux.

@akien-mga
Copy link
Member

Ah I think I misunderstood what type clients is. It's a HashMap, so it uses arbitrary keys and not contiguous indices, so clients.get(1) might be valid if size is 1 and the only key is 1. The error checks should use !clients.has(p_client_id).

@akien-mga akien-mga self-assigned this Jun 15, 2020
akien-mga added a commit to akien-mga/godot that referenced this issue Jun 15, 2020
@akien-mga
GDScript LSP: Fix wrong error checks added in godotengine#39385
786f4ad 
Reverts `latest_client_id` back to 0, as I misunderstood how the client
IDs are assigned and, without further testing and debugging, I can't
say if this was a bug or a valid default value.
Similarly, a `latest_client_id` of -1 is no longer raising an error.

Fixes godotengine#39548.
@akien-mga akien-mga mentioned this issue Jun 15, 2020
Merged
akien-mga added a commit that referenced this issue Jun 15, 2020
@akien-mga
GDScript LSP: Fix wrong error checks added in #39385
93e20a4 
Reverts `latest_client_id` back to 0, as I misunderstood how the client
IDs are assigned and, without further testing and debugging, I can't
say if this was a bug or a valid default value.
Similarly, a `latest_client_id` of -1 is no longer raising an error.

Fixes #39548.

(cherry picked from commit 786f4ad)
ChristopheLY pushed a commit to ChristopheLY/godot that referenced this issue Jun 22, 2020
@akien-mga @ChristopheLY
GDScript LSP: Fix wrong error checks added in godotengine#39385
337430a 
Reverts `latest_client_id` back to 0, as I misunderstood how the client
IDs are assigned and, without further testing and debugging, I can't
say if this was a bug or a valid default value.
Similarly, a `latest_client_id` of -1 is no longer raising an error.

Fixes godotengine#39548.
huhund pushed a commit to huhund/godot that referenced this issue Nov 10, 2020
@akien-mga
GDScript LSP: Fix wrong error checks added in godotengine#39385
bee2d9b 
Reverts `latest_client_id` back to 0, as I misunderstood how the client
IDs are assigned and, without further testing and debugging, I can't
say if this was a bug or a valid default value.
Similarly, a `latest_client_id` of -1 is no longer raising an error.

Fixes godotengine#39548.

(cherry picked from commit 786f4ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@akien-mga akien-mga

Labels
bug regression topic:gdscript
Projects
None yet
Milestone
4.0
Development

Successfully merging a pull request may close this issue.

GDScript LSP: Fix wrong error checks added in #39385 akien-mga/godot
2 participants
@akien-mga @Rubonnek