Skip to content

Fix crash in JSON/HTML LSP servers when initializationOptions are not provided#107775

Merged
aeschli merged 2 commits intomicrosoft:masterfrom
apexskier:handle-missing-initialization-options
Sep 30, 2020
Merged

Fix crash in JSON/HTML LSP servers when initializationOptions are not provided#107775
aeschli merged 2 commits intomicrosoft:masterfrom
apexskier:handle-missing-initialization-options

Conversation

@apexskier
Copy link
Copy Markdown
Contributor

@apexskier apexskier commented Sep 30, 2020

A bug hasn't been filed, but this somewhat relates to #72124.

I'm using the VSCode json language server with another LSP client (Nova.app), and am seeing crashes in the server because it's assuming the initialization request contains initializationOptions. The spec says they're optional and there's some handling of them as optional, so I think this is just and oversight and a clear bug.

I've gone through usages of initializationOptions for the JSON, CSS, and HTML servers and updated all of them to consistently treat them as optional.

I've tested this manually with my Nova extension. I did not see any unit tests related to this functionality to add to, and I think it's a minimal enough change that I'm not planning to add any.

@ghost
Copy link
Copy Markdown

ghost commented Sep 30, 2020

CLA assistant check
All CLA requirements met.

@aeschli
Copy link
Copy Markdown
Contributor

aeschli commented Sep 30, 2020

Looks good! Let me know whet the PR is ready (it's still marked as draft)

@apexskier
Copy link
Copy Markdown
Contributor Author

I think it's ready, I was just waiting for the build (and went off doing other things)

@apexskier apexskier marked this pull request as ready for review September 30, 2020 10:35
@aeschli aeschli merged commit 2a7b5d3 into microsoft:master Sep 30, 2020
@aeschli aeschli added this to the September 2020 milestone Sep 30, 2020
@aeschli
Copy link
Copy Markdown
Contributor

aeschli commented Sep 30, 2020

Thanks @apexskier !

@apexskier apexskier deleted the handle-missing-initialization-options branch September 30, 2020 17:10
@apexskier
Copy link
Copy Markdown
Contributor Author

Thanks for merging, does anyone know what the publishing schedule/process for this package is? (Asking because it doesn't seem to be published on the same cadence as vscode itself. The last version was published 7 months ago, so three are quite a few pending changes)

@aeschli
Copy link
Copy Markdown
Contributor

aeschli commented Oct 1, 2020

It's done on request :-)
I just published [email protected]
If you can give it a try that would be great

@apexskier
Copy link
Copy Markdown
Contributor Author

apexskier commented Oct 2, 2020

Dang, there's a crashing issue on startup with the new release due to bad file references. I saw this locally as well, but fixed it locally as I thought it was due to some magic happening at release time to make this cross platform.

#107949 I'll see what I can find

@github-actions github-actions Bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants