search

TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 205 forks source link

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

Closed ayame113 closed 2 years ago

ayame113 commented 3 years ago

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. (https://github.com/TypeStrong/atom-typescript/commit/19a483f096b1596ae823462fde56234280337e3a)

  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. (https://github.com/TypeStrong/atom-typescript/commit/c631eddeba19220503bdfacf17da4a9f021e172e)

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Ah, the atom-ide-ui package returns disposable from the consumer, but the atom-ide-base package returns nothing from the consumer. This may be the cause.

https://github.com/facebookarchive/atom-ide-ui/blob/710f8477607d3788aeadcbdd55079925241ce40d/modules/atom-ide-ui/pkg/atom-ide-outline-view/lib/main.js#L48-L50

https://github.com/atom-community/atom-ide-outline/blob/642c9ebbc5de7ca8124f388fe12198b84476d91c/src/main.ts#L53-L64

lierdakil commented 3 years ago

Thanks for investigating!

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

/cc @aminya

aminya commented 3 years ago

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

lierdakil commented 3 years ago

@aminya, please read the conversation, specifically https://github.com/TypeStrong/atom-typescript/pull/1587#issuecomment-869009474. 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 commented 3 years ago

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 https://github.com/atom-community/atom-ide-outline/issues/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 555b5ff115714229020f7d2a4a02a0298688f02f though.

Let me know if you agree this can be closed.

aminya commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 2 years ago

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

ayame113 commented 2 years ago

OK, let's close this PR. Thanks!