[READY] Rely on connect timeout instead of checking that the server is alive #2514
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
Rely on connect timeout instead of checking if the server is alive.
Current coverage is 85.50% (diff: 100%)@@ master #2514 diff @@
==========================================
Files 19 19
Lines 1980 1966 -14
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 1687 1681 -6
+ Misses 293 285 -8
Partials 0 0
|
|
I'm fine in general with this idea, I'm only worried that 10ms is a too little as a timeout 😕 did you try it? did you ever encountered any problem? Review status: 0 of 4 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
|
I love this change! Deletes code, uses a more elegant approach, improves startup, prevents spamming the user on timeouts... fantastic! Well done! @vheon 10ms is not the total call timeout, it's the "establish a TCP handshake" timeout. The time to establish that handshake should be well within 10ms on a local ycmd (and we don't support non-local ycmd). Review status: 0 of 4 files reviewed at latest revision, all discussions resolved. Comments from Reviewable |
Then Reviewed 4 of 4 files at r1. Comments from Reviewable |
|
📌 Commit 8426191 has been approved by |
homu
added a commit
that referenced
this pull request
Jan 26, 2017
[READY] Rely on connect timeout instead of checking that the server is alive Currently, we always check that the ycmd process is up (with the `IsServerAlive` method) before sending a request. Without this check, each request could block Vim until a `NewConnectionError` exception is raised if the server crashed. This is the case on Windows where it takes ~1s before the exception is raised which makes Vim unusable. However, even with this check, Vim may still be blocked in the following cases: - the server crashes just after the check but before sending the request; - the server is up but unresponsive (e.g. its port is closed). To avoid both cases, we instead use [the connect timeout parameter from Requests](http://docs.python-requests.org/en/master/user/advanced/?highlight=connect%20timeout#timeouts) and set it to a duration sufficiently short (10 ms) that the blocking can't be noticed by the user. Since the server is supposed to run locally (this is what YCM is designed for), 10ms is largely enough to establish a connection. The `IsServerAlive` check is removed almost everywhere except in `OnFileReadyToParse` because we still want to notify the user if the server crashed. This change makes it possible to not have to [wait for the server to be healthy before sending asynchronous requests](https://github.com/Valloric/YouCompleteMe/blob/master/python/ycm/client/base_request.py#L137-L138). This will dramatically improve startup time (see issue #2085) and fixes #2071. Next PR once this one is merged. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2514) <!-- Reviewable:end -->
|
☀️ Test successful - status |
homu
added a commit
that referenced
this pull request
Jan 28, 2017
[READY] Remove healthy check This PR removes the code that, if the server is not yet healthy, tries in a separate thread to send the same request at different intervals until it gets a response or reaches 5 failed attempts. Main issue with that code is that it blocks Vim if a synchronous request (`semantic_completion_available`, `run_completer_command`, etc.) is sent while the server is not up. See issue #2071. Other issues are that it's hard to understand how this code behaves (it combines threads and retries), it becomes useless once the server is healthy (because `SERVER_HEALTHY` is cached and never reset), it depends on arbitrary parameters (why 5 retries with a delay of 0.5s multiplied by 1.5 at each attempt?), and it has a package dependency `retries`. Now that we catch all server exceptions (PR #2453) and define a very short connection timeout (PR #2514), we can just drop this code. This change gives a huge improvement in startup time on Windows (don't worry, more improvements are coming for all platforms). The reason is that `requests` (probably `urllib3`) tries for ~1s to check if the server is healthy on Windows while it's almost instantaneous on other platforms. <table> <tr> <th rowspan="2">Platform</th> <th colspan="2">First run (ms)</th> <th colspan="2">Subsequent runs (ms)</th> </tr> <tr> <td>Before</td> <td>After</td> <td>Before</td> <td>After</td> </tr> <tr> <td>Ubuntu 16.04 64-bit</td> <td>228</td> <td>248</td> <td>176</td> <td>151</td> </tr> <tr> <td>macOS 10.12</td> <td>433</td> <td>417</td> <td>263</td> <td>263</td> </tr> <tr> <td>Windows 10 64-bit</td> <td>1861</td> <td>844</td> <td>814</td> <td>292</td> </tr> </table> These results were obtained by running the `prof.py` script from [this branch](https://github.com/micbou/YouCompleteMe/tree/profiling-startup). The difference between first run and subsequent runs is Python bytecode generation (`*.pyc` files). Fixes #2071. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2517) <!-- Reviewable:end -->
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.
Currently, we always check that the ycmd process is up (with the
IsServerAlivemethod) before sending a request. Without this check, each request could block Vim until aNewConnectionErrorexception is raised if the server crashed. This is the case on Windows where it takes ~1s before the exception is raised which makes Vim unusable. However, even with this check, Vim may still be blocked in the following cases:To avoid both cases, we instead use the connect timeout parameter from Requests and set it to a duration sufficiently short (10 ms) that the blocking can't be noticed by the user. Since the server is supposed to run locally (this is what YCM is designed for), 10ms is largely enough to establish a connection.
The
IsServerAlivecheck is removed almost everywhere except inOnFileReadyToParsebecause we still want to notify the user if the server crashed.This change makes it possible to not have to wait for the server to be healthy before sending asynchronous requests. This will dramatically improve startup time (see issue #2085) and fixes #2071. Next PR once this one is merged.
This change is