Refactor SphinxManager and SubprocessSphinxClient
#744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a method that allows components to ask the configuration system which configuration scope a given uri would fall into
By pushing the execution of async configuration change handlers into `asyncio.Task` objects, we can make the `subscribe()` method syncronous. Not only does this not force users of the function to be async, we now actually detect duplicate subscriptions and prevent unecessary calls to the given handler.
This commit refactors the `SubprocessSphinxClient` so that it's hopefully much more robust. - There is no longer a separation between starting the client and creating a Sphinx application. There is now only a single `start()` method. - The `start()` method no longer takes any arguments (the `SphinxConfig` is passed to the constructor instead) and the method returns `self` - The client is now `await`-able. If required the `start()` method will be called and the client simply returned otherwise. - There is now a `ClientState` enum which better explains the current condition of the client. - The client is now an `EventSource` and will emit a `state-change` event whenever the state is changed.
This commit builds on the changes introduced to the `SubprocessSphinxClient` to greatly simplify and improve the robustness of managing the set of `SphinxClients` in use by the language server. There were a few drawbacks to the previous approach - While await on the `_client_creating` task during client creation appeared to work, it still often led to the creation of duplicated clients. - By not recognizing when a given configuration was broken, the manager would continually attempt to spawn client instances. - It was not uncommon to enter an infinite loop of spawning clients! The new system introduced in this commit *shouldn't* suffer from any of the above issues. - Instead of indexing clients by `src_uri`, they are instead indexed by the scope of the configuration governing the instance. While still not perfect, we should be able to accurately determine the relevant client for a given uri in more scenarios. - To indicate that we are unable to create a client for a given config we now store a `None` in the clients dictionary - preventing any further attempts at spawning a client. - Taking advantage of the fact clients are indexed by scope and that the client itself is `await`-able, we can have multiple consumers of the client awaiting on the client - preventing the issue of duplicated clients. Currently, there is a quirk to the system where the first batch of callers to `get_client` in a given scope will be told there is no client. However, they have in fact "primed" the system by creating a config subscription which will asynchronously populate the `clients` dict with an unstarted client. The next batch of callers will then discover that there is a client instance available and `await` it, starting the underlying Sphinx process. Based on some initial testing this "quirk" may actually be desirable, the `get_client` method is called often enough that I wasn't able to notice a real delay in starting the Sphinx process up. It also has the nice property that when restarting the server with multiple projects open, the flood of `didOpen` notifications from the client is enough to "prime" the system for each project, but the project itself is only activated when working on the relevant file in the editor. One final note, there are plenty of improvements still to be made - error handling, config changes etc, but hopefully this is a solid base we can start building on.
The tests now use the real thing and there does not seem to be much benefit in trying to maintain this.
A number of tests have been marked as xfail or skipped as they will be fixed for real in the next batch of changes
alcarney
force-pushed
the
client-refactor
branch
from
f871260 to
d2a4256
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refactors the management of underlying Sphinx processes to be much simpler and hopefully much more robust.
See commit messages for 58d3727 and 340bea5 for full details
Closes #660