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

Pyright doesn't work anymore for unsaved buffers #281

Closed
jwortmann opened this issue Nov 9, 2023 · 15 comments · Fixed by sublimelsp/LSP#2360
Closed

Pyright doesn't work anymore for unsaved buffers #281

jwortmann opened this issue Nov 9, 2023 · 15 comments · Fixed by sublimelsp/LSP#2360

Comments

@jwortmann
Copy link
Member

It seems that LSP-pyright is now broken for unsafed buffers, for which the server is enabled by default. After you created such an unsafed buffer, the server simply doesn't respond to requests anymore, both for the buffer scheme and also for regular files, until it is restarted. It looks like as if an internal crash happend in the server.

https://github.com/sublimelsp/LSP-pyright/releases/tag/1.2.52 (pyright 1.1.333) seems to be the first affected version.

@jfcherng
Copy link
Collaborator

jfcherng commented Nov 9, 2023

xxx.mp4

I can see :: [20:21:57.454] --> LSP-pyright shutdown (2): None. At this moment everything goes well.

But then Window(2) no longer enables the server for unsaved buffer.

@jfcherng
Copy link
Collaborator

jfcherng commented Nov 9, 2023

I noticed that pyright doesn't response to :: [20:21:57.454] --> LSP-pyright shutdown (2): None in the above screen recording.

May related to microsoft/pyright#6314

I can see pyright immediately takes 100% CPU of a core, only if the server process is invoked by a unsaved buffer.

@jwortmann
Copy link
Member Author

I assume that it must be a server bug. It's not just the shutdown request, there are no more responses to any request and it doesn't send publishDiagnostic notifications anymore. So it's as if the server simply died after receiving a didOpen notification with buffer URI scheme.

I looked at a few commits in pyright between 1.1.332 and 1.1.333, and this one might be a possible cause because it seems to affect URI handling: microsoft/pyright@cfe56bf (just speculation).

Perhaps something breaks for the buffer scheme now, because such a scheme wasn't considered since VS Code uses untitled for unsafed buffers. But a strange thing is that Pyright still works fine for the res scheme (from View Package File), so I might be wrong with that assumption.

Yeah, your linked issue looks like it is related.

@jfcherng
Copy link
Collaborator

jfcherng commented Nov 9, 2023

VSCode's Pylance and Pyright extension doesn't suffer from 100% CPU (with unsaved buffer).

@rchl
Copy link
Member

rchl commented Nov 9, 2023

sublimelsp/LSP#2360

It was never able to return from this loop: https://github.com/microsoft/pyright/blob/a95a808e580f81bad6f35196b784b6b6fe13d8f7/packages/pyright-internal/src/analyzer/importResolver.ts#L508-L587

The buffer://sublime/... URI would never look like a "root" when trying to traverse that "path".

@jwortmann
Copy link
Member Author

sublimelsp/LSP#2360

While it's fine to remove the sublime part in the URI, it should still be a valid URI even with that part, so I would say this is clearly a server bug and nothing that could be fixed in LSP.

The buffer://sublime/... URI would never look like a "root" when trying to traverse that "path".

The URIs for "View Package File" look like res://Packages/..., which is kind of similar, so it's unclear to me why the bug doesn't occur for those views if this would be the only cause.


Even with the PR merged I can still reproduce the server "dying" with the example from sublimelsp/LSP#2360 (comment). It happens right when you type import in an unsaved buffer.

So I guess this issue could be reopened, and for now I would suggest to remove the "buffer" in

"buffer", // in-memory buffers
until it gets fixed in the server.

@rchl
Copy link
Member

rchl commented Nov 10, 2023

Pyright does something to the effect of file traversing that walks "up the tree" that causes this issue. Though not sure why would it do that for non-file URIs. Best to file an issue with it.

@jwortmann
Copy link
Member Author

Wait, actually I wonder whether those are really valid URIs. I think buffer://42 is not a valid URI, because the path (here //42) must not begin with //.

https://datatracker.ietf.org/doc/html/rfc3986#section-3.3

If a URI does not contain an authority component, then the path cannot begin with two slash characters ("//").

@rchl
Copy link
Member

rchl commented Nov 10, 2023

Good point.

>>> import urllib;urllib.parse.urlparse('buffer://xx')
ParseResult(scheme='buffer', netloc='xx', path='', params='', query='', fragment='')

(netloc being the authority in this implementation, I believe)

That doesn't make the URI invalid, I don't think. It's just that the view id is parsed as authority here. And I suppose we don't really care about the authority/path part so it shouldn't matter for us.

So we could switch to buffer:view_id so that view id is parsed as path if we'd want that.

Note that vscode uses untitled:foo and the spec, while doesn't explicitly specify it, does mention the untitled: scheme in a couple of comments. But that would be a breaking change for us.

@jfcherng
Copy link
Collaborator

But that would be a breaking change for us.

We can use "buffer" in plugin settings, but sents "untitled:foo" under the hood. Or deprecates "buffer" and introduces "untitled".

@jwortmann
Copy link
Member Author

That doesn't make the URI invalid, I don't think. It's just that the view id is parsed as authority here. And I suppose we don't really care about the authority/path part so it shouldn't matter for us.

Ah, you're right, the path is empty in this case and the view_id will become the authority. While being a valid URI, I think it would be better to use an empty authority and non-empty path.

I would keep the buffer scheme, but adjust the URIs like this:

buffer:42

Perhaps this already fixes the bug with unsaved buffers in pyright.

I'm unsure about the res scheme, where for example Packages/Default/symbol.py looks like a path. But currently Packages will be the authority and /Default/symbol.py would be the path. Probably it makes more sense to leave authority empty here too and make Packages to be part of the path, like

res:Packages/Default/symbol.py

But on the other hand ST seems to already use res:// for the <img> tag in minihtml (https://www.sublimetext.com/docs/minihtml.html#tags), so maybe we should not care and leave it like that.

jfcherng added a commit that referenced this issue Nov 10, 2023
@jfcherng
Copy link
Collaborator

So I guess this issue could be reopened, and for now I would suggest to remove the "buffer" in

"buffer", // in-memory buffers

until it gets fixed in the server.

Done, with a release tagged.

@jfcherng jfcherng reopened this Nov 10, 2023
@jfcherng
Copy link
Collaborator

jfcherng commented Nov 15, 2023

Hi all, just want to know this is a pyright issue or a LSP issue (or both? or unclear yet?).

Pyright's maintainer is asking for a "100% CPU consumption" reproduction on microsoft/pyright#6314 (comment)

@jwortmann jwortmann changed the title Pyright doesn't work anymore for unsafed buffers Pyright doesn't work anymore for unsaved buffers Nov 15, 2023
@jfcherng
Copy link
Collaborator

jfcherng commented Nov 22, 2023

This seems to be fixed in the latest pyright pyright 1.1.337 (without LSP's change).

@rwols
Copy link
Member

rwols commented Nov 22, 2023

In the release notes, I see:

Fixed bug that led to a hang (infinite loop) in the language server when opening a document whose URI wasn't a "file". This occurred in some language servers that used an "untitled" (or similar) URI type for new documents.

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 a pull request may close this issue.

4 participants