purescript-emacs / psc-ide-emacs

Emacs integration for PureScript's psc-ide tool.
GNU General Public License v3.0
132 stars 31 forks source link

Smarter server offline detection #169

Closed kritzcreek closed 5 years ago

kritzcreek commented 5 years ago

There are various workflows that don't start the ide server from within Emacs, so I'd like to support those too. For example:

I've wanted to do this for a long time, so thank you @purcell (if you could take a look at this change that would be great!) for finally getting me to do it :D I've just got one request, please add a short note to CHANGELOG.md if you make a change that changes the plugins behaviour in a visible way.

purcell commented 5 years ago

I'd suggest keeping a psc-ide-server-running-p function rather than switching everything to a direct variable check, because if that logic changes later then it will be a pain to fix all the usages.

Right now psc-ide-protocol.el keeps making network connections for every request. It occurs to me that we should perhaps just have a "connection" buffer in which we keep a persistent network process. Then to determine if the server is running in psc-ide-server-running-p, we'd check for the liveness of that network process, and if we found ourselves to be unconnected we could prompt to reconnect and/or start the server inside Emacs. This scheme is more in line with how other language server backends are typically implemented.

purcell commented 5 years ago

(Additionally, maybe what we're talking about here is more like psc-ide-connected-p than psc-ide-server-running-p.)

kritzcreek commented 5 years ago

because if that logic changes later then it will be a pain to fix all the usages

I'd agree if the variable was read from in more than two places. I'll pull it out into a function once I see the need to add more usages, as then I'll also have a better idea of what that function is abstracting over.

It occurs to me that we should perhaps just have a "connection" buffer in which we keep a persistent network process.

There are various reasons for why purs ide doesn't use persistent connections.

  1. I don't need to handle request-concurrency in the ide server, while still supporting multiple consumers for the same server.
  2. Managing connections is resource management which is always tricky
  3. The main motivation for a persistent connection model would be the ability to have a stateful protocol, but I much prefer our stateless one because it's just simpler and makes testing a lot easier. (This is not entirely true, there is a tiny bit of state in that the last rebuilt model has its export list deleted so we can complete module internal qualifiers)
kritzcreek commented 5 years ago

because if that logic changes later then it will be a pain to fix all the usages

I'd agree if the variable was read from in more than two places. I'll pull it out into a function once I see the need to add more usages, as then I'll also have a better idea of what that function is abstracting over.

Oh wow, I remembered that completely wrong, there's more usages. I'll extract the function :D

purcell commented 5 years ago

There are various reasons for why purs ide doesn't use persistent connections.

All that makes a lot of sense.

kritzcreek commented 5 years ago

Thanks for taking a look!

purcell commented 5 years ago

This has broken my workflow, because I'd gone to some trouble to set things up in psc-ide so that the Flycheck backend would degrade gracefully if the server was not running. This meant - importantly, I think - that the user could open a bunch of Purescript buffers with psc-ide-mode and flycheck enabled, and they could choose to start the server later, at which point Flycheck would start reporting errors.

With this PR, this behaviour has regressed, and errors are thrown when typing in a buffer before the server has been started.

Perhaps the problem is as simple as the fact that you're defaulting psc-ide-server-running to t when psc-ide-mode is started?

kritzcreek commented 5 years ago

The error should only be thrown once, and become silent from there on out. That's how it works for me at least.

purcell commented 5 years ago

Yes, but it's once per buffer. As a general rule errors shouldn't routinely occur -- I run with debug-on-error enabled to catch this sort of thing, so this admittedly accentuates the issue for me!

I'll have a look and see what options there are while retaining the more flexible behaviour you've enabled here.

purcell commented 5 years ago

I'll explain what I found out and what I've done here, but you should be busy celebrating! :tada:

Some of what's going on is that the maintenance of the psc-ide-server-running var was partly taking place in code like this. That code set the variable buffer-locally inside a temporary buffer that was then deleted, so it had no effect. For this to work, the variable would have needed to be maintained either in the source buffer or - ideally - in a single place visible to all of the buffers in the project, e.g. a *psc-ide: localhost:4242* buffer.

In the end I've committed what I think is a much simpler approach in 2d76406, and commented there on the trade-offs. That commit should allow things to work seamlessly for both of our use cases. In the event that you like getting a prominent error to remind you to start your server, we could consider: a) printing a message when enabling psc-ide-mode if the server is not available, and/or b) making the ! in the modeline more prominent, e.g. by using [STOPPED] instead. Let me know what you think, and whether you find any issues! :-)