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

Failure to find Node gets cached #116

Open
ISSOtm opened this issue Jan 10, 2024 · 6 comments
Open

Failure to find Node gets cached #116

ISSOtm opened this issue Jan 10, 2024 · 6 comments

Comments

@ISSOtm
Copy link

ISSOtm commented Jan 10, 2024

LSP-json was failing to find a suitable Node.js (I had 12, welcome to Ubuntu LTS :P); but even after installing 18, the same error kept appearing. I traced this to the caching being done in NodeRuntime, and fixed it by restarting Sublime Text.

@rchl
Copy link
Member

rchl commented Jan 10, 2024

It's unlikely that it will be addressed. I think the benefits of caching are bigger than correctly handling a corner cases like that.

@ISSOtm
Copy link
Author

ISSOtm commented Jan 10, 2024

Well, AIUI, changing cls._node_runtime_resolved = True to cls._node_runtime_resolved = cls._node_runtime is not None (modulo any Python subtleties I'm not aware of) should be a simple enough change not to drop much benefit.

As for whether such a setup qualifies as a "corner case", please let me disagree on that; LTS distributions are still very common, and in this behaviour made debugging very difficult for me.

  1. I got the initial exception window, which offered to install Node; but I refused, because I want to use my system package manager to avoid winrot.
  2. So I ran apt install nodejs, and got the exact same issue after issuing the suggested LSP: Enable server in project command.
  3. After looking into this repo's issues, I also ran apt install npm, and still no dice.
  4. Only then did I look into the console, but there was no backtrace beyond "no Node environment found"—despite me having just installed once, and only followed instructions given on-screen thus far.
  5. Once I went and looked into the source code did I realise that this caching was going on, and tried opening Sublime Text.
  6. Only when LSP-json is trying to start for the first time do you get a full lookup backtrace, giving notably the crucial detail that my Node runtime was outdated.

Considering that this is a bog-standard Ubuntu 20.10 LTS install, which I believe is a pretty popular distribution, I don't think it's worth dismissing as "edge case". (And while I've been using the command line, one would've gotten the same results from using the graphical application managers, since they basically issue apt commands as their backend.)

@rchl
Copy link
Member

rchl commented Jan 10, 2024

I suppose not caching when resolving failed would be OK. Though then someone could still come and report an issue that the we don't pick up newly updated node version due to caching.

@ISSOtm
Copy link
Author

ISSOtm commented Jan 11, 2024

Yeah, but that's exactly the tradeoff made by caching (“there are two hard things in programming...”). I suppose that could be mitigated by refreshing the cache if it's old enough (i.e. cls._node_runtime_resolved would become cls._time_node_runtime_resolved. That starts veering into more complexity though (only updating that timestamp on resolution success, notably), so I'd understand if you decline to implement it.

Alternatively, if you can think of a way to allow the user to request a cache flush, then that would be a non-issue. But I'm not sure that's a possibility at all, considering how “internal”/low-level this library is.

@rchl
Copy link
Member

rchl commented Jan 21, 2024

Also I remembered now that the caching is a way to avoid the user being spammed with multiple error dialogs when there are multiple servers installed or multiple windows opened.

@ISSOtm
Copy link
Author

ISSOtm commented Feb 10, 2024

I want to suggest some way to "deduplicate" those error messages then, but something like a timeout is fairly arbitrary and "magic" thus hinders debugging.

Somehow printing a notice that the returned failure comes from a cache, with an indication to restart ST will refresh it, would make everyone happy?

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

No branches or pull requests

2 participants