Auto merge of #2514 - micbou:connect-timeout, r=Valloric

[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 -->

autoload/youcompleteme.vim

@@ -735,9 +735,6 @@ function! youcompleteme#Complete( findstart, base )
       return -2
     endif
 
-    if !s:Pyeval( 'ycm_state.IsServerAlive()' )
-      return -2
-    endif
     exec s:python_command "ycm_state.CreateCompletionRequest()"
     return s:Pyeval( 'base.CompletionStartColumn()' )
   else
@@ -748,9 +745,6 @@ endfunction
 
 function! youcompleteme#OmniComplete( findstart, base )
   if a:findstart
-    if !s:Pyeval( 'ycm_state.IsServerAlive()' )
-      return -2
-    endif
     let s:omnifunc_mode = 1
     exec s:python_command "ycm_state.CreateCompletionRequest(" .
           \ "force_semantic = True )"

python/ycm/client/base_request.py

@@ -40,8 +40,9 @@ from ycmd.responses import ServerError, UnknownExtraConf
 
 _HEADERS = {'content-type': 'application/json'}
 _EXECUTOR = UnsafeThreadPoolExecutor( max_workers = 30 )
+_CONNECT_TIMEOUT_SEC = 0.01
 # Setting this to None seems to screw up the Requests/urllib3 libs.
-_DEFAULT_TIMEOUT_SEC = 30
+_READ_TIMEOUT_SEC = 30
 _HMAC_HEADER = 'x-ycm-hmac'
 _logger = logging.getLogger( __name__ )
 
@@ -67,7 +68,7 @@ class BaseRequest( object ):
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
   @staticmethod
-  def GetDataFromHandler( handler, timeout = _DEFAULT_TIMEOUT_SEC ):
+  def GetDataFromHandler( handler, timeout = _READ_TIMEOUT_SEC ):
     return JsonFromFuture( BaseRequest._TalkToHandlerAsync( '',
                                                             handler,
                                                             'GET',
@@ -78,7 +79,7 @@ class BaseRequest( object ):
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
   @staticmethod
-  def PostDataToHandler( data, handler, timeout = _DEFAULT_TIMEOUT_SEC ):
+  def PostDataToHandler( data, handler, timeout = _READ_TIMEOUT_SEC ):
     return JsonFromFuture( BaseRequest.PostDataToHandlerAsync( data,
                                                                handler,
                                                                timeout ) )
@@ -88,7 +89,7 @@ class BaseRequest( object ):
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
   @staticmethod
-  def PostDataToHandlerAsync( data, handler, timeout = _DEFAULT_TIMEOUT_SEC ):
+  def PostDataToHandlerAsync( data, handler, timeout = _READ_TIMEOUT_SEC ):
     return BaseRequest._TalkToHandlerAsync( data, handler, 'POST', timeout )
 
 
@@ -100,7 +101,7 @@ class BaseRequest( object ):
   def _TalkToHandlerAsync( data,
                            handler,
                            method,
-                           timeout = _DEFAULT_TIMEOUT_SEC ):
+                           timeout = _READ_TIMEOUT_SEC ):
     def SendRequest( data, handler, method, timeout ):
       request_uri = _BuildUri( handler )
       if method == 'POST':
@@ -111,12 +112,12 @@ class BaseRequest( object ):
             headers = BaseRequest._ExtraHeaders( method,
                                                  request_uri,
                                                  sent_data ),
-            timeout = timeout )
+            timeout = ( _CONNECT_TIMEOUT_SEC, timeout ) )
       if method == 'GET':
         return BaseRequest.session.get(
             request_uri,
             headers = BaseRequest._ExtraHeaders( method, request_uri ),
-            timeout = timeout )
+            timeout = ( _CONNECT_TIMEOUT_SEC, timeout ) )
 
     @retries( 5, delay = 0.5, backoff = 1.5 )
     def DelayedSendRequest( data, handler, method ):
@@ -222,6 +223,11 @@ def HandleServerException( display = True, truncate = False ):
         _LoadExtraConfFile( e.extra_conf_file )
       else:
         _IgnoreExtraConfFile( e.extra_conf_file )
+  except requests.exceptions.ConnectTimeout:
+    # We don't display this exception to the user since it is likely to happen
+    # for each subsequent request (typically if the server crashed) and we
+    # don't want to spam the user with it.
+    _logger.exception( 'Unable to connect to server' )
   except Exception as e:
     _logger.exception( 'Error while handling server response' )
     if display:

python/ycm/tests/youcompleteme_test.py

@@ -171,7 +171,7 @@ def YouCompleteMe_DebugInfo_ServerNotRunning_test( ycm ):
       ycm.DebugInfo(),
       matches_regexp(
         'Client logfile: .+\n'
-        'Server crashed, no debug info from server\n'
+        'Server errored, no debug info from server\n'
         'Server running at: .+\n'
         'Server process ID: \d+\n'
         'Server logfiles:\n'

python/ycm/youcompleteme.py

@@ -254,8 +254,7 @@ class YouCompleteMe( object ):
 
 
   def _ShutdownServer( self ):
-    if self.IsServerAlive():
+    SendShutdownRequest()
-      SendShutdownRequest()
 
 
   def RestartServer( self ):
@@ -298,15 +297,13 @@ class YouCompleteMe( object ):
 
 
   def SendCommandRequest( self, arguments, completer ):
-    if self.IsServerAlive():
+    return SendCommandRequest( arguments, completer )
-      return SendCommandRequest( arguments, completer )
 
 
   def GetDefinedSubcommands( self ):
-    if self.IsServerAlive():
+    with HandleServerException():
-      with HandleServerException():
+      return BaseRequest.PostDataToHandler( BuildRequestData(),
-        return BaseRequest.PostDataToHandler( BuildRequestData(),
+                                            'defined_subcommands' )
-                                              'defined_subcommands' )
     return []
 
 
@@ -324,9 +321,6 @@ class YouCompleteMe( object ):
     except KeyError:
       pass
 
-    if not self.IsServerAlive():
-      return False
-
     exists_completer = SendCompleterAvailableRequest( filetype )
     if exists_completer is None:
       return False
@@ -363,22 +357,16 @@ class YouCompleteMe( object ):
 
 
   def OnBufferUnload( self, deleted_buffer_file ):
-    if not self.IsServerAlive():
-      return
     SendEventNotificationAsync( 'BufferUnload', filepath = deleted_buffer_file )
 
 
   def OnBufferVisit( self ):
-    if not self.IsServerAlive():
-      return
     extra_data = {}
     self._AddUltiSnipsDataIfNeeded( extra_data )
     SendEventNotificationAsync( 'BufferVisit', extra_data = extra_data )
 
 
   def OnInsertLeave( self ):
-    if not self.IsServerAlive():
-      return
     SendEventNotificationAsync( 'InsertLeave' )
 
 
@@ -399,8 +387,6 @@ class YouCompleteMe( object ):
 
 
   def OnCurrentIdentifierFinished( self ):
-    if not self.IsServerAlive():
-      return
     SendEventNotificationAsync( 'CurrentIdentifierFinished' )
 
 
@@ -633,8 +619,6 @@ class YouCompleteMe( object ):
 
 
   def ShowDetailedDiagnostic( self ):
-    if not self.IsServerAlive():
-      return
     with HandleServerException():
       detailed_diagnostic = BaseRequest.PostDataToHandler(
           BuildRequestData(), 'detailed_diagnostic' )
@@ -648,10 +632,7 @@ class YouCompleteMe( object ):
     debug_info = ''
     if self._client_logfile:
       debug_info += 'Client logfile: {0}\n'.format( self._client_logfile )
-    if self.IsServerAlive():
+    debug_info += FormatDebugInfoResponse( SendDebugInfoRequest() )
-      debug_info += FormatDebugInfoResponse( SendDebugInfoRequest() )
-    else:
-      debug_info += 'Server crashed, no debug info from server\n'
     debug_info += (
       'Server running at: {0}\n'
       'Server process ID: {1}\n'.format( BaseRequest.server_location,
@@ -669,8 +650,8 @@ class YouCompleteMe( object ):
                       self._server_stdout,
                       self._server_stderr ]
 
-    if self.IsServerAlive():
+    debug_info = SendDebugInfoRequest()
-      debug_info = SendDebugInfoRequest()
+    if debug_info:
       completer = debug_info[ 'completer' ]
       if completer:
         for server in completer[ 'servers' ]: