tide icon indicating copy to clipboard operation
tide copied to clipboard

Only start one tsserver, not one per tsconfig

Open sandersn opened this issue 6 years ago • 11 comments

This is how tsserver is intended to be used, according to Typescript team members who work on it. This improves the usability of a long-lived emacs instance after opening many different projects.

I'm testing the change right now and so far performance is better. I don't know whether there are deep-seated reasons to have one tsserver per project root, so I thought that we could discuss that with this PR.

I'm not sure whether initialising tide-server with defvar to nil and updating it with setq are right since I don't know elisp that well.

sandersn avatar May 18 '18 18:05 sandersn

Historically tide only supported working against one particular version of tsserver (shipped with tide or overridden, custom path set by the user). In such circumstances, this change would make sense.

Now however, tide defaults to using the same tsserver as provided by the typescript npm package installed in each and every project it works with.

This ensures tide handles things consistently with the projects specific version of tsc, the typescript compiler, automatically.

This was a “big” (although fairly silent) feature with lots of discussion and code-review in the PRs.

Having only one instance of tsserver would directly invalidate all that work invested in that undertaking, so that’s not immediately desired, at least not for all users.

While I can see the argument for preserving resources, it would have to be under very specific constraints, like always using a globally installed tsserver as opposed to the projects’ own.

josteink avatar May 18 '18 18:05 josteink

What about one tsserver per tsserver location instead of one per project? tide-servers could use tide-locate-tsserver-executable to get the hash key instead of tide-project-name.

sandersn avatar May 18 '18 19:05 sandersn

I have some repos that have multiple tsconfig.json files. Often, there's one for the application proper, and one for running the test suite. Something like

src/foo.ts
src/tsconfig.json
test/foo.spec.ts
test/tsconfig.json

Way back when, tsserver did not play well with this kind of setup. If I started editing src/foo.ts and then moved to edit test/foo.spec.ts, then I would get behavior out of tsserver that was consistent with it incorrectly using src/tsconfig.json for processing test/foo.spec.ts instead of correctly using test/tsconfig.json. And if I opened the files in the reverse order then src/foo.ts was not handled properly.

This, in combination with issue reports like this one (this comment, specifically), gave me the impression that tsserver is not designed to handle multiple tsconfig.json files at the same time.

lddubeau avatar May 18 '18 20:05 lddubeau

@lddubeau Are you saying the current behaviour of hashing tsservers on tide-project-name is better or worse than hashing on tide-locate-tsserver-executable? Or would the bug you linked be the same no matter how many tsservers you start?

sandersn avatar May 18 '18 20:05 sandersn

@sandersn The current behavior of hashing on tide-project-name is better than hashing on tide-locate-tsserver-executable. Hashing on the location of tsserver in the scenario I gave above would result in a single tsserver handling source files that are governed by different tsconfig.json files, and in my experience, that did not work.

Hashing on tide-project-name means one instance of tsserver per tsconfig.json. tide-project-name looks up the file system for a tsconfig.json and then takes the basename of the directory that contains the tsconfig.json and appends a hash to it to work around clashes. In the example I gave above, there would be two project names: src-[some hash] and test-[some other hash], and two instances of tsserver.

lddubeau avatar May 18 '18 20:05 lddubeau

Assuming that tsserver still has the same problem from Microsoft/TypeScript#5828, maybe we should wait until Typescript supports dependencies between tsconfigs, which should be coming in 3.0 or 3.1. That feature exists to make src and test relate correctly.

To recap, it seems like there are two reasons to start one tsserver per tsconfig.

  1. Support local installs of typescript.
  2. Work around src-vs-test-interaction bugs in tsserver.

If/when (2) is fixed, then I think hashing on tide-locate-tsserver-executable would be more efficient and therefore strictly more desirable, right?

sandersn avatar May 18 '18 20:05 sandersn

@sandersn Yep, when the 2nd point is fixed it will be better to hash per location to avoid starting so many instances.

lddubeau avatar May 18 '18 20:05 lddubeau

@lddubeau Also, if it's been a long time since you observed the bug, it may have been fixed. You can test it by using a constant hash key like "DUMMY" in the gethash line in tide-current-server and the puthash line in tide-start-server.

I realise it's an imposition to ask you to test whether a bug has been fixed, but I'd like to solve real bugs now (for me, running out of file handles when running VS code at the same time as Emacs) instead of avoiding old bugs that have been fixed.

sandersn avatar May 18 '18 21:05 sandersn

We could introduce a config tide-reuse-tsserver and set it to nil by default until all the bugs are sorted out in tsserver. If enabled, it would search for existing tsserver process with same path and reuse it.

Codewise, we assume one server would be used by one project in some places like tide-restart-server, tide-net-sentinel. We also store the project-name in process.

(process-put process 'project-name (tide-project-name))

I guess we could convert it into project-names and also add another process property tsserver-path which could be used to find server for reuse.

ananthakumaran avatar May 19 '18 02:05 ananthakumaran

@sandersn I tried it just now. It is still buggy. Initially, it looks like it is working but I soon get bizarre behavior, like hitting 100% CPU on tsserver until I kill it. It all goes away when I revert to tide's stock behavior.

@ananthakumaran Yes, I was thinking it should be configurable. It is very likely that users of tide won't be able to move all their projects at once to the new version of TS that has a tsserver that handles multiple projects without error. These users will want to use the current method of starting tsserver until they no longer have to deal with the old versions.

lddubeau avatar May 20 '18 15:05 lddubeau

Further experimentation with this branch revealed bad behaviour for my Typescript-compiler-development scenario as well (details below). I don't have time for investigation now, so I guess I'll live with the resource usage.

  1. Point tsserver to require("~/ts/built/local/tsserver.js")
  2. Open two projects: ~/ts/src/compiler and ~/test. The locally-built tsserver starts once, for the first project.
  3. Rebuild the tsserver at ~/ts/built/local/tsserver.
  4. Switch to a source file in ~/ts/src/compiler and request the type of something. The minibuffer shows part of a stack trace.
  5. tide-restart-server. Now tsserver is working in ~/ts/src/compiler.
  6. Switch to a source file in ~/test and request the type of something. The minibuffer again shows part of a stack trace.
  7. tide-restart-server. No tsserver is working in ~/test.
  8. Goto 4 or give up.

sandersn avatar May 24 '18 15:05 sandersn