WIP: Remove server lifecycle management from EnsimeClient #367
Conversation
| if not server.isrunning(): | ||
| self.start_server(config) | ||
| client.connect(server) | ||
| client.connection_attempts += 1 |
There was a problem hiding this comment.
I'm just preserving how the prior implementation behaved so far, but while we're looking at this, it seems excessive to me to check this on every CursorMove—CursorHold is probably fine, but CursorMove a bit much. The server and our connection are not so unstable. Might experiment with removing the call to this from au_cursor_moved.
There was a problem hiding this comment.
I think experimentation won't hurt and if it turns out to be unnecessary I am more than welcome to remove it. However, there is a tradeoff here between handlers and responsiveness. If me remove CursorHold, ensime-vim could turn less responsive and responses may be processed with delays. I hope this is not the case though, I would very much prefer to use only one -- but let's keep in mind that if we're browsing through a file and waiting for some reaction of the plugin, this won't happen until the cursor is on hold for a few secs.
There was a problem hiding this comment.
I think ches meant remove just the reconnection logic from CursorHold, if unqueue_and_display is left there we won't lose any responsiveness.
There was a problem hiding this comment.
Yes indeed, the queue processing remains on CursorMoved, but checking that server is up and connected is now removed and it seems fine to me.
| @@ -300,3 +362,13 @@ def fun_en_complete_func(self, client, findstart_and_base, base=None): | |||
| # Invoked by vim | |||
| findstart = findstart_and_base | |||
| return client.complete_func(findstart, base) | |||
|
|
|||
| def watchdog_client_server_connection(self, client): | |||
There was a problem hiding this comment.
This isn't working (for me), but don't worry, I will take a look as part of #350
There was a problem hiding this comment.
Hmm, what about it isn't working?
There was a problem hiding this comment.
Dunno if it should support this case, but I tried killing the server processes and the client goes to a bad state just logging websocket not connected continuously. I have to restart vim session.
But this is related to my PR where I have to make some changes so recover from a disconnection is possible.
There was a problem hiding this comment.
Ah okay yeah, I hadn't gone that far but now I have while trying to determine if something else was a bug… the latest commit probably gets you even closer (it does some of the same things you started doing on #350 in fact) but I think that EnsimeProcess has some wrong behavior that needs to be fixed in order for Server to work right when it has a stale reference to a process. I might leave that for you to pick up in the interest of keeping the scope in check here.
ensime_shared/client.py
Outdated
| self.suggestions = None | ||
| self.completion_timeout = 10 # seconds | ||
| self.completion_started = False | ||
|
|
||
| self.full_types_enabled = False | ||
| """Whether fully-qualified types are displayed by inspections or not""" | ||
|
|
||
| self.toggle_teardown = True | ||
| self.connection_attempts = 0 | ||
| self.tmp_diff_folder = tempfile.mkdtemp(prefix='ensime-vim-diffs') | ||
|
|
||
| # By default, don't connect to server more than once |
There was a problem hiding this comment.
Does this comment still make sense here after removing the field?
There was a problem hiding this comment.
No it doesn't, thanks for pointing that out—I still want to make this stuff clearer. connection_retries was a rename from number_try_connection because it was clearer to me for how it was used, but it's still confusing that the connect method and queue_poll are based on counting down connection_retries, then what is now watchdog_client_server_connection counts up connection_attempts. I'm guessing that at one time the former was meant to be like a config value for max retries, and the count-up should use that instead of a hardcoded literal, but it perhaps got mixed up over time.
I was pretty much translating the existing code on these up until now, but I'll consolidate them.
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
0029e6b to
ee7ab2e
Compare
There was a problem hiding this comment.
I really like these changes. Big up @ches. Happy to merge at any time. I wanted to chat with you guys to speed up the merge process -- to me any small improvement to the codebase can be merged right away, we don't really need PRs implementing complete features in order to click the merge button. So long as every commit works and passes the tests, it's good to go 😄.
README.md
Outdated
| @@ -1,7 +1,6 @@ | |||
| # Ensime for vim `ensime-vim` | |||
| # `ensime-vim` – ENSIME with Vim! | |||
There was a problem hiding this comment.
I would suggest something like "ensime-vim -- Scala support in Vim!". I sometimes feel that people don't really get what ensime is about and don't realize what this plugin is capable of. Making it explicit (though redundant) may help attract users as well as contributors.
There was a problem hiding this comment.
Agree that makes sense, improve the SEO juju 😄 I'll change this.
| bootstrap_server=False, | ||
| create_client=True): | ||
| """Decorator that gets a client and performs an operation on it.""" | ||
| def execute_with_client(): |
There was a problem hiding this comment.
I'm happy to see these args go away!
There was a problem hiding this comment.
Yeah these args and the related EnsimeClient.setup method, probably my happiest moments of making these changes 😁 🍾
ensime_shared/ensime.py
Outdated
| return client | ||
| key = os.path.realpath(config_path) | ||
| if key in self.clients: | ||
| return self.clients[key] |
There was a problem hiding this comment.
Do you think not using return may improve readability? I usually avoid several returns because they break program logic in unexpected ways and they have a big effect on readability in big chunks of code (as readers lose the thread of the logic and can be easily missed). Though redundant, I usually prefer this style:
client = None
if key in self.clients:
client = self.clients[key]
else:
client = self.create_client(config_path)
return clientThe previous code snippet is very common in Java code, where the idiom of "only return once" is quite used. I'm not aware of Python devs using it and I don't want to bikeshed --all this refactoring is amazing--, but I'm just curious what you think about it.
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
7e655b0 to
cce7c92
Compare
|
|
||
| self.log = setup_logger() | ||
| self.log.debug('__init__: in') | ||
| self.editor.initialize() | ||
|
|
||
| self.ws = None | ||
| self.ensime = None |
There was a problem hiding this comment.
This change breaks :EnClients
There was a problem hiding this comment.
Indeed, I was aware but hadn't left a todo comment, thanks for logging it here.
:EnClients is currently undocumented, I think it should be renamed to :EnStatus to reflect that it's really as much about server statuses, and the output formatting enhanced. This could even become a special filetype window for a "console" in our server lifecycle plans, with some summarized key mappings for performing actions like restarting the server on the current line, etc.
I'll just make some minimal fix here so it isn't utterly broken, until we give EnsimeProcess some attention in forthcoming work.
There was a problem hiding this comment.
I think that :EnStatus would be a better name and it's better to tackle this in another PR.
ensime_shared/ensime.py
Outdated
| """Handler for CursorHold autocommand event.""" | ||
| self.watchdog_client_server_connection(client) | ||
| client.unqueue_and_display(filename) | ||
| client.editor.cursorhold() |
There was a problem hiding this comment.
This call to editor's cursorhold causes the event to be retriggered every second when no activity is happening. I don't think this is the intended behaviour.
There was a problem hiding this comment.
This was moved as-is from EnsimeClient.on_cursor_hold so it's not a behavior change in this PR. I don't exactly understand the background of how this achieves "faking a timer" as the link referenced at Editor.cursohold describes, but if there's anything wrong with that I'd propose it's brought up in a new issue.
There was a problem hiding this comment.
I've been using this PR for the last days. It works well, though I've discovered that vim is slower to respond to keystrokes and, sometimes, events are fired by no reason. For instance, when typing code, the UI gets blocked. I infer this happens because ensime-vim is querying the server and waiting for its response. I don't know if this is a consequence of what @ktonga points out.
There was a problem hiding this comment.
Okay, my issue is that I have a deoplete delay of 450ms that was fired all the time. I removed it. I'll file an issue to keep track of the "blocked UI" issue, as I've experienced it in other ensime requests that take a lot of time to come back.
There was a problem hiding this comment.
Deoplete while using omnifunc does wait for the response in a synchronous way, and more often than I'd like ensime server gets really really slow to respond having to wait several seconds after each keystroke what makes a terrible typing experience, that's why I want to implemente a deoplete source so the integration with autocomplete will be asynchronous avoiding those horrible blocks. But we have to work on ensime-vim integration API first.
There was a problem hiding this comment.
Thanks for testing @jvican. Would be curious about reproducible cases of blocking the UI if it's not completions, I think they're probably not results of this PR, but still.
In theory these changes could improve responsiveness: there's a subtle change where I moved the time.sleep in the polling thread's loop, it was running on every normal iteration and upon inspection this is needless, socket recv is efficient blocking I/O that's exactly what we want to happen with no artificial pause for this threading model. I moved the sleep to instead prevent a total busy-loop in the condition that we've lost the connection. This reconnection stuff isn't perfect yet but we'll focus on that soon; I'm not positive that there aren't race conditions (i.e. the running and connected attributes shared between threads without a queue), but these things haven't really changed here—a deep refactoring of this logic wasn't my intention, yet. I haven't actually been able to observe what I think is a race, the GIL may be solving it. If we're noticeably blocking Vim anywhere though, that's got to be the main thread.
I've been traveling this week so haven't had a chance to tie up the loose ends on this PR, will try this weekend.
There was a problem hiding this comment.
It's isn't our fault this time, we're only the middleman here. The responsibility for the blocking is shared between Ensime server which from time to time likes taking a nap before responding, and the naive omnicompletion API Vim provides (and we hook into) which is called synchronously from (I guess since it blocks) the UI thread.
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
02032bc to
d54b7d3
Compare
|
Hey @ches do you have any outstanding work on this PR to be pushed? I can rebase it and merge it if you don't have the time, I think it is a huge improvement over the global design that could unblock a bunch of other improvements and I'd like to get it merged sooner rather than later. Cheers. |
|
I do have quite a bit more based on top of this that's sitting WIP actually, and I've been using it for awhile too. I'll try to see what's needed to wrap it up cleanly, won't be able to until after Monday but hopefully can find some time by mid-week. Perhaps for one thing we can simplify on the caveat noted in the PR description by just going ahead and dropping support for the old server bootstrap installation in this branch—we're 8+ months into sbt-ensime handling that, I'm sure just about everyone is using it now. |
|
Totally, let's stop supporting legacy stuff, it is easy enough for people to keep sbt up to date since the recommended plugin installation is add sbt-ensime as a local/per-developer plugin. I think waiting a few more days for it after waiting for several months is ok :) |
|
Rebasing this has been a bit tricky after the timer change from #374 and plus we're on server v2 by default now, etc. I have it working but it's shakier than it was before these changes. I think I'll finish sorting it out today though. |
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
d54b7d3 to
cd0e8c7
Compare
|
So, I've pushed rebased commits, but the status is still basically as I noted in my last comment: it has some issues that weren't present before. It mostly works, you should be able to run it if you want to try to help me debug, but I'm seeing a few problems:
The interaction of this branch's changes with the timer introduced in #374 is tricky and I've probably just missed reconciling something correctly when rebasing. I'll keep poking at it to try to fix the issues, if anyone tries it now and sees where problems are coming from do let me know! |
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
cd0e8c7 to
ad5045b
Compare
At one time these were to support shutting down the server, but had become inconsistent and broken. The fact that the toggle_teardown attribute of EnsimeClient had a default value of True is probably a hint... We're working toward more robust support for controlling server lifecycle now.
I was looking at and updating the overview here and thought it'd make sense if it was actually expressed in the source :-) http://ensime.github.io/editors/vim/contributing/#ensime-integration
Killing the server doesn't completely lead to everything restarting yet, but it's almost there. Might leave the rest to @ktonga on #350. Generally it's supposed to be an invariant that if client's self.connected is true, then self.ws should be set--so use that everywhere instead of having a mix that's confusing.
This allows an Editor instance to be an attribute of the Ensime class since it avoids use of vim.eval during Ensime's constructor, where that is a known initialization problem. Side effects in constructors often lead to trouble so this is probably the right thing to do anyway.
ches
force-pushed
the
remove-server-lifecycle-from-client
branch
from
ad5045b to
90a7da5
Compare
3 participants
This resolves #325, that ticket serves as a good statement of the purpose of this PR.
I still have a few known todos here and I'm working on writing some tests, but this is such a large and core change (and frankly one that I'm really eager to get out of the way) that I wanted to open it up and get some people trying it. It basically works for me now but I'd like to know if anything turns out to be fragile.
Caveats at present to highlight and spare you from trying:
:EnClientscommand (client_statusPython function) is broken by the these changes. Should be easily fixable, I just didn't bother with it yet. As discussed below I'd like to change it to a:EnStatuscommand with more features in the future—will create a follow-up issue for this.It won't start a bootstrapped server installation. Please use current sbt-ensime that installs the server.
This stems from the
Serverabstraction. I don't want it to know aboutvim(because how does that make any sense?), butEnsimeLaunchercurrently needsvimas a shell runner for bootstrap installations. As discussed inline we might just drop the server bootstrap functionality now on this branch, it's now been 8+ months since sbt-ensime handled server installation for you, if other build tools don't do it yet they should probably get feature requests filed.