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

Fix the error that occurs when disable and then enable #1587

Closed
wants to merge 2 commits into from

Conversation

ayame113
Copy link
Contributor

  • All compiled assets are included (atom packages are git tags and hence the built files need to be a part of the source control)

When I enable and then disable this package I get the following error:

"Error processing request. No Project.
Error: No Project.
    at Object.ThrowNoProject (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:152133:23)
    at IOSession.Session.getFileAndLanguageServiceForSyntacticOperation (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160157:42)
    at IOSession.Session.getNavigationTree (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160516:31)
    at Session.handlers.ts.Map.ts.getEntries._a.<computed> (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:159306:61)
    at C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160963:88
    at IOSession.Session.executeWithRequestId (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160954:28)
    at IOSession.Session.executeCommand (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160963:33)
    at IOSession.Session.onMessage (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:160989:35)
    at Interface.<anonymous> (C:\Users\azusa\work\Atom\atom-typescript\node_modules\typescript\lib\tsserver.js:163650:31)
    at Interface.emit (events.js:223:5)
    at Interface._onLine (readline.js:315:10)
    at Interface._normalWrite (readline.js:460:12)
    at Socket.ondata (readline.js:172:10)
    at Socket.emit (events.js:223:5)
    at addChunk (_stream_readable.js:309:12)
    at readableAddChunk (_stream_readable.js:290:11)
    at Socket.Readable.push (_stream_readable.js:224:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:181:23)"

This pull request wants to fix this.

  1. Delete the cache of TypescriptEditorPane and TypescriptBuffer at the time of invalidation, to prevent that use the old tsserver at the time of re-enable. (19a483f)

  2. Re-enabling this package will provide "providedServices" again. The old "providedServices" cannot be disabled, so it conflicts with the new one and causes an error. Therefore, raise the "Priority" property of the newer to prevent conflicts. (c631edd)

With the above changes, I think that this is adapted to function as expected.
This is my first contribution to this repository, so please let me know if there are any fixes.

@lierdakil
Copy link
Collaborator

lierdakil commented Jun 26, 2021

Hmm. This seems like a very elaborate way of sidestepping poor shutdown... Definitely not a good idea to change priorities, because those may (likely will) have unexpected interactions with other packages.

I didn't investigate the issue in any detail, but it seems the root of the issue is that either TypescriptBuffers are not cleaned-up correctly, or that tsserver client doesn't shut down properly. Maybe worth investigation.

IDE-UI services should be disposed properly either internally by Atom or on the consumer side. What UI packages are you using? I see no errors with (admittedly no longer supported) atom-ide-ui.

@ayame113
Copy link
Contributor Author

Thank you for seeing this.
As far as I can see, the tsserver is shut down, but I'm getting an error because a request was sent to the shut down tsserver.

The package I'm using is the one included in atom-ide-base.

@ayame113
Copy link
Contributor Author

@lierdakil
Copy link
Collaborator

Thanks for investigating!

I think the service disposal issue is a bug with atom-ide-outline then (and maybe others?)

/cc @aminya

@aminya
Copy link
Contributor

aminya commented Jun 26, 2021

This fixes #1550. I will look into the code.

@lierdakil
Copy link
Collaborator

@aminya, please read the conversation, specifically #1587 (comment). The code in this PR, tbf, has issues, but the investigation is great. The "no project" issue I'll fix a bit more carefully in a bit (the code in this PR leaves dangling subscriptions, regrettably, so it's a half-measure). The service disposal needs to be fixed on atom-ide-outline end, hence the ping.

@lierdakil
Copy link
Collaborator

I've (hopefully) fixed the "no project" error in v14.3.2. Also added some miscellaneous clean-up when the package is disabled (it did have a habit of leaving dangling tsservers running, apparently).

@aminya please check if #1550 is reproducible with v14.3.2 (since I couldn't reproduce that one, and you didn't provide a detailed reproduction guide still, I can't really check). Also please check if atom-community/atom-ide-outline#128 has any relation to #1550

@ayame113, thank you very much for the investigation you did here. I'm sorry I didn't end up using any of your code. I did add you as a co-author on 555b5ff though.

Let me know if you agree this can be closed.

@aminya
Copy link
Contributor

aminya commented Jun 27, 2021

Sorry, I hadn't read the details once I left my previous comment. I can fix the outline issue in a future version of the outline. Is this urgent, or is it fine to defer it for a week or so?

@lierdakil
Copy link
Collaborator

Is this emergent, or is it fine to defer it for a week or so?

Well, as outlined in the OP, the issue only manifests when outline provider package is disabled and re-enabled, so I don't think it's urgent. Most users don't disable and re-enable packages often, and the state is fixed with Atom restart -- that's probably why the issue escaped notice until now. @ayame113, feel free to correct me if I'm wrong.

@ayame113
Copy link
Contributor Author

@lierdakil
Thank you for fixing it.

I checked other provideServices and found that atom-ide-definitions have similar issues. Fixing atom-ide-outline and atom-ide-definitions should fix this issue completely.

@aminya
I'm not in a hurry to fix this.
But, I just looked it up, so I think I can immediately send a PR to atom-ide-outline and atom-ide-definitions. Can I do it?

@lierdakil
Copy link
Collaborator

@ayame113 as far as I can gather, the issue has been fixed upstream a while back. Is it okay to close this PR?

@ayame113
Copy link
Contributor Author

OK, let's close this PR. Thanks!

@ayame113 ayame113 closed this Oct 28, 2021
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 this pull request may close these issues.

3 participants