Auto merge of #2976 - micbou:refactor-server-exception-handling, r=puremourning

[READY] Refactor server exception handling

Having to wrap all request calls with the `HandleServerException` function is inconvenient. Handle server exceptions directly in `BaseRequest`.

<!-- 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/2976)
<!-- Reviewable:end -->

python/ycm/client/base_request.py

@@ -22,7 +22,6 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-import contextlib
 import logging
 import json
 import vim
@@ -58,28 +57,71 @@ class BaseRequest( object ):
   def Response( self ):
     return {}
 
+
+  @staticmethod
+  def HandleFuture( future, display_message = True, truncate_message = False ):
+    """Get the server response from a |future| object and catch any exception
+    while doing so. If an exception is raised because of a unknown
+    .ycm_extra_conf.py file, load the file or ignore it after asking the user.
+    For other exceptions, log the exception and display its message to the user
+    on the Vim status line. Unset the |display_message| parameter to hide the
+    message from the user. Set the |truncate_message| parameter to avoid
+    hit-enter prompts from this message."""
+    try:
+      try:
+        return _JsonFromFuture( future )
+      except UnknownExtraConf as e:
+        if vimsupport.Confirm( str( e ) ):
+          _LoadExtraConfFile( e.extra_conf_file )
+        else:
+          _IgnoreExtraConfFile( e.extra_conf_file )
+    except BaseRequest.Requests().exceptions.ConnectionError:
+      # 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_message:
+        DisplayServerException( e, truncate_message )
+
+    return None
+
+
   # This method blocks
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
+  # See the HandleFuture method for the |display_message| and |truncate_message|
+  # parameters.
   @staticmethod
-  def GetDataFromHandler( handler, timeout = _READ_TIMEOUT_SEC ):
+  def GetDataFromHandler( handler,
-    return JsonFromFuture( BaseRequest._TalkToHandlerAsync( '',
+                          timeout = _READ_TIMEOUT_SEC,
-                                                            handler,
+                          display_message = True,
-                                                            'GET',
+                          truncate_message = False ):
-                                                            timeout ) )
+    return BaseRequest.HandleFuture(
+        BaseRequest._TalkToHandlerAsync( '', handler, 'GET', timeout ),
+        display_message,
+        truncate_message )
 
 
   # This is the blocking version of the method. See below for async.
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
+  # See the HandleFuture method for the |display_message| and |truncate_message|
+  # parameters.
   @staticmethod
-  def PostDataToHandler( data, handler, timeout = _READ_TIMEOUT_SEC ):
+  def PostDataToHandler( data,
-    return JsonFromFuture( BaseRequest.PostDataToHandlerAsync( data,
                          handler,
-                                                               timeout ) )
+                         timeout = _READ_TIMEOUT_SEC,
+                         display_message = True,
+                         truncate_message = False ):
+    return BaseRequest.HandleFuture(
+        BaseRequest.PostDataToHandlerAsync( data, handler, timeout ),
+        display_message,
+        truncate_message )
 
 
-  # This returns a future! Use JsonFromFuture to get the value.
+  # This returns a future! Use HandleFuture to get the value.
   # |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
@@ -87,7 +129,7 @@ class BaseRequest( object ):
     return BaseRequest._TalkToHandlerAsync( data, handler, 'POST', timeout )
 
 
-  # This returns a future! Use JsonFromFuture to get the value.
+  # This returns a future! Use HandleFuture to get the value.
   # |method| is either 'POST' or 'GET'.
   # |timeout| is num seconds to tolerate no response from server before giving
   # up; see Requests docs for details (we just pass the param along).
@@ -185,7 +227,7 @@ def BuildRequestData( buffer_number = None ):
   }
 
 
-def JsonFromFuture( future ):
+def _JsonFromFuture( future ):
   response = future.result()
   _ValidateResponseObject( response )
   if response.status_code == BaseRequest.Requests().codes.server_error:
@@ -200,43 +242,6 @@ def JsonFromFuture( future ):
   return None
 
 
-@contextlib.contextmanager
-def HandleServerException( display = True, truncate = False ):
-  """Catch any exception raised through server communication. If it is raised
-  because of a unknown .ycm_extra_conf.py file, load the file or ignore it after
-  asking the user. Otherwise, log the exception and display its message to the
-  user on the Vim status line. Unset the |display| parameter to hide the message
-  from the user. Set the |truncate| parameter to avoid hit-enter prompts from
-  this message.
-
-  The GetDataFromHandler, PostDataToHandler, and JsonFromFuture functions should
-  always be wrapped by this function to avoid Python exceptions bubbling up to
-  the user.
-
-  Example usage:
-
-   with HandleServerException():
-     response = BaseRequest.PostDataToHandler( ... )
-  """
-  try:
-    try:
-      yield
-    except UnknownExtraConf as e:
-      if vimsupport.Confirm( str( e ) ):
-        _LoadExtraConfFile( e.extra_conf_file )
-      else:
-        _IgnoreExtraConfFile( e.extra_conf_file )
-  except BaseRequest.Requests().exceptions.ConnectionError:
-    # 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:
-      DisplayServerException( e, truncate )
-
-
 def _LoadExtraConfFile( filepath ):
   BaseRequest.PostDataToHandler( { 'filepath': filepath },
                                  'load_extra_conf_file' )
@@ -247,14 +252,14 @@ def _IgnoreExtraConfFile( filepath ):
                                  'ignore_extra_conf_file' )
 
 
-def DisplayServerException( exception, truncate = False ):
+def DisplayServerException( exception, truncate_message = False ):
   serialized_exception = str( exception )
 
   # We ignore the exception about the file already being parsed since it comes
   # up often and isn't something that's actionable by the user.
   if 'already being parsed' in serialized_exception:
     return
-  vimsupport.PostVimMessage( serialized_exception, truncate = truncate )
+  vimsupport.PostVimMessage( serialized_exception, truncate = truncate_message )
 
 
 def _ToUtf8Json( data ):

python/ycm/client/command_request.py

@@ -22,8 +22,7 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+from ycm.client.base_request import BaseRequest, BuildRequestData
-                                      HandleServerException )
 from ycm import vimsupport
 from ycmd.utils import ToUnicode
 
@@ -53,7 +52,6 @@ class CommandRequest( BaseRequest ):
       'completer_target': self._completer_target,
       'command_arguments': self._arguments
     } )
-    with HandleServerException():
     self._response = self.PostDataToHandler( request_data,
                                              'run_completer_command' )
 

python/ycm/client/completer_available_request.py

@@ -22,8 +22,7 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+from ycm.client.base_request import BaseRequest, BuildRequestData
-                                      HandleServerException )
 
 
 class CompleterAvailableRequest( BaseRequest ):
@@ -36,7 +35,6 @@ class CompleterAvailableRequest( BaseRequest ):
   def Start( self ):
     request_data = BuildRequestData()
     request_data.update( { 'filetypes': self.filetypes } )
-    with HandleServerException():
     self._response = self.PostDataToHandler( request_data,
                                              'semantic_completion_available' )
 

python/ycm/client/completion_request.py

@@ -22,20 +22,21 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
+import logging
 from future.utils import iteritems
 from ycmd.utils import ToUnicode
-from ycm.client.base_request import ( BaseRequest, JsonFromFuture,
+from ycm.client.base_request import ( BaseRequest, DisplayServerException,
-                                      HandleServerException,
                                       MakeServerException )
 from ycm import vimsupport
 
+_logger = logging.getLogger( __name__ )
+
 
 class CompletionRequest( BaseRequest ):
   def __init__( self, request_data ):
     super( CompletionRequest, self ).__init__()
     self.request_data = request_data
     self._response_future = None
-    self._response = { 'completions': [], 'completion_start_column': -1 }
     self._complete_done_hooks = {
       'cs': self._OnCompleteDone_Csharp,
       'java': self._OnCompleteDone_Java,
@@ -53,19 +54,22 @@ class CompletionRequest( BaseRequest ):
 
   def RawResponse( self ):
     if not self._response_future:
-      return self._response
+      return { 'completions': [], 'completion_start_column': -1 }
 
-    with HandleServerException( truncate = True ):
+    response = self.HandleFuture( self._response_future,
-      self._response = JsonFromFuture( self._response_future )
+                                  truncate_message = True )
+    if not response:
+      return { 'completions': [], 'completion_start_column': -1 }
 
     # Vim may not be able to convert the 'errors' entry to its internal format
     # so we remove it from the response.
-      errors = self._response.pop( 'errors', [] )
+    errors = response.pop( 'errors', [] )
     for e in errors:
-        with HandleServerException( truncate = True ):
+      exception = MakeServerException( e )
-          raise MakeServerException( e )
+      _logger.error( exception )
+      DisplayServerException( exception, truncate_message = True )
 
-    return self._response
+    return response
 
 
   def Response( self ):

python/ycm/client/debug_info_request.py

@@ -22,8 +22,7 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+from ycm.client.base_request import BaseRequest, BuildRequestData
-                                      HandleServerException )
 
 
 class DebugInfoRequest( BaseRequest ):
@@ -37,8 +36,9 @@ class DebugInfoRequest( BaseRequest ):
     request_data = BuildRequestData()
     if self._extra_data:
       request_data.update( self._extra_data )
-    with HandleServerException( display = False ):
+    self._response = self.PostDataToHandler( request_data,
-      self._response = self.PostDataToHandler( request_data, 'debug_info' )
+                                             'debug_info',
+                                             display_message = False )
 
 
   def Response( self ):

python/ycm/client/event_notification.py

@@ -22,8 +22,7 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+from ycm.client.base_request import BaseRequest, BuildRequestData
-                                      JsonFromFuture, HandleServerException )
 
 
 class EventNotification( BaseRequest ):
@@ -57,8 +56,8 @@ class EventNotification( BaseRequest ):
     if not self._response_future or self._event_name != 'FileReadyToParse':
       return []
 
-    with HandleServerException( truncate = True ):
+    self._cached_response = self.HandleFuture( self._response_future,
-      self._cached_response = JsonFromFuture( self._response_future )
+                                               truncate_message = True )
 
     return self._cached_response if self._cached_response else []
 

python/ycm/client/messages_request.py

@@ -22,11 +22,9 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
+from ycm.client.base_request import BaseRequest, BuildRequestData
 from ycm.vimsupport import PostVimMessage
 
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
-                                      JsonFromFuture, HandleServerException )
-
 import logging
 
 _logger = logging.getLogger( __name__ )
@@ -65,8 +63,11 @@ class MessagesPoll( BaseRequest ):
       # Nothing yet...
       return True
 
-    with HandleServerException( display = False ):
+    response = self.HandleFuture( self._response_future,
-      response = JsonFromFuture( self._response_future )
+                                  display_message = False )
+    if response is None:
+      # Server returned an exception.
+      return False
 
     poll_again = _HandlePollResponse( response, diagnostics_handler )
     if poll_again:

python/ycm/client/shutdown_request.py

@@ -22,7 +22,7 @@ from __future__ import absolute_import
 # Not installing aliases from python-future; it's unreliable and slow.
 from builtins import *  # noqa
 
-from ycm.client.base_request import BaseRequest, HandleServerException
+from ycm.client.base_request import BaseRequest
 
 TIMEOUT_SECONDS = 0.1
 
@@ -33,8 +33,10 @@ class ShutdownRequest( BaseRequest ):
 
 
   def Start( self ):
-    with HandleServerException( display = False ):
+    self.PostDataToHandler( {},
-      self.PostDataToHandler( {}, 'shutdown', TIMEOUT_SECONDS )
+                            'shutdown',
+                            TIMEOUT_SECONDS,
+                            display_message = False )
 
 
 def SendShutdownRequest():

python/ycm/client/ycmd_keepalive.py

@@ -24,7 +24,7 @@ from builtins import *  # noqa
 
 import time
 from threading import Thread
-from ycm.client.base_request import BaseRequest, HandleServerException
+from ycm.client.base_request import BaseRequest
 
 
 # This class can be used to keep the ycmd server alive for the duration of the
@@ -45,5 +45,4 @@ class YcmdKeepalive( object ):
     while True:
       time.sleep( self._ping_interval_seconds )
 
-      with HandleServerException( display = False ):
+      BaseRequest.GetDataFromHandler( 'healthy', display_message = False )
-        BaseRequest.GetDataFromHandler( 'healthy' )

python/ycm/omni_completer.py

@@ -26,7 +26,7 @@ import vim
 from ycm import vimsupport
 from ycmd import utils
 from ycmd.completers.completer import Completer
-from ycm.client.base_request import BaseRequest, HandleServerException
+from ycm.client.base_request import BaseRequest
 
 OMNIFUNC_RETURNED_BAD_VALUE = 'Omnifunc returned bad value to YCM!'
 OMNIFUNC_NOT_LIST = ( 'Omnifunc did not return a list or a dict with a "words" '
@@ -124,7 +124,6 @@ class OmniCompleter( Completer ):
       'query': query
     }
 
-    with HandleServerException():
+    response = BaseRequest.PostDataToHandler( request_data,
-      return BaseRequest.PostDataToHandler( request_data,
                                               'filter_and_sort_candidates' )
-    return candidates
+    return response if response is not None else candidates

python/ycm/tests/completion_test.py

@@ -42,20 +42,17 @@ def MockCompletionRequest( response_method ):
   """Mock out the CompletionRequest, replacing the response handler
   JsonFromFuture with the |response_method| parameter."""
 
-  # We don't want the event to actually be sent to the server, just have it
+  # We don't want the requests to actually be sent to the server, just have it
   # return success.
+  with patch( 'ycm.client.completer_available_request.'
+              'CompleterAvailableRequest.PostDataToHandler',
+              return_value = True ):
     with patch( 'ycm.client.completion_request.CompletionRequest.'
                 'PostDataToHandlerAsync',
                 return_value = MagicMock( return_value=True ) ):
 
-    # We set up a fake response (as called by CompletionRequest.RawResponse)
+      # We set up a fake response.
-    # which calls the supplied callback method.
+      with patch( 'ycm.client.base_request._JsonFromFuture',
-    #
-    # Note: JsonFromFuture is actually part of ycm.client.base_request, but we
-    # must patch where an object is looked up, not where it is defined.
-    # See https://docs.python.org/dev/library/unittest.mock.html#where-to-patch
-    # for details.
-    with patch( 'ycm.client.completion_request.JsonFromFuture',
                   side_effect = response_method ):
         yield
 
@@ -83,11 +80,8 @@ def SendCompletionRequest_UnicodeWorkingDirectory_test( ycm ):
 
 
 @YouCompleteMeInstance()
-@patch( 'ycm.client.base_request._logger', autospec = True )
 @patch( 'ycm.vimsupport.PostVimMessage', new_callable = ExtendedMock )
-def SendCompletionRequest_ResponseContainingError_test( ycm,
+def SendCompletionRequest_ResponseContainingError_test( ycm, post_vim_message ):
-                                                        post_vim_message,
-                                                        logger ):
   current_buffer = VimBuffer( 'buffer' )
 
   def ServerResponse( *args ):
@@ -117,8 +111,6 @@ def SendCompletionRequest_ResponseContainingError_test( ycm,
       ycm.SendCompletionRequest()
       ok_( ycm.CompletionRequestReady() )
       response = ycm.GetCompletionResponse()
-      logger.exception.assert_called_with( 'Error while handling server '
-                                           'response' )
       post_vim_message.assert_has_exact_calls( [
         call( 'Exception: message', truncate = True )
       ] )

python/ycm/tests/event_notification_test.py

@@ -78,16 +78,11 @@ def MockEventNotification( response_method, native_filetype_completer = True ):
               'PostDataToHandlerAsync',
               return_value = MagicMock( return_value=True ) ):
 
-    # We set up a fake response (as called by EventNotification.Response) which
+    # We set up a fake a Response (as called by EventNotification.Response)
-    # calls the supplied callback method. Generally this callback just raises an
+    # which calls the supplied callback method. Generally this callback just
-    # apropriate exception, otherwise it would have to return a mock future
+    # raises an apropriate exception, otherwise it would have to return a mock
-    # object.
+    # future object.
-    #
+    with patch( 'ycm.client.base_request._JsonFromFuture',
-    # Note: JsonFromFuture is actually part of ycm.client.base_request, but we
-    # must patch where an object is looked up, not where it is defined.  See
-    # https://docs.python.org/dev/library/unittest.mock.html#where-to-patch for
-    # details.
-    with patch( 'ycm.client.event_notification.JsonFromFuture',
                 side_effect = response_method ):
 
       # Filetype available information comes from the server, so rather than

python/ycm/tests/youcompleteme_test.py

@@ -337,7 +337,7 @@ def YouCompleteMe_ToggleLogs_WithoutParameters_NoSelection_test(
 def YouCompleteMe_GetDefinedSubcommands_ListFromServer_test( ycm ):
   current_buffer = VimBuffer( 'buffer' )
   with MockVimBuffers( [ current_buffer ], current_buffer ):
-    with patch( 'ycm.client.base_request.JsonFromFuture',
+    with patch( 'ycm.client.base_request._JsonFromFuture',
                 return_value = [ 'SomeCommand', 'AnotherCommand' ] ):
       assert_that(
         ycm.GetDefinedSubcommands(),
@@ -356,7 +356,7 @@ def YouCompleteMe_GetDefinedSubcommands_ErrorFromServer_test( ycm,
                                                               logger ):
   current_buffer = VimBuffer( 'buffer' )
   with MockVimBuffers( [ current_buffer ], current_buffer ):
-    with patch( 'ycm.client.base_request.JsonFromFuture',
+    with patch( 'ycm.client.base_request._JsonFromFuture',
                 side_effect = ServerError( 'Server error' ) ):
       result = ycm.GetDefinedSubcommands()
 
@@ -374,7 +374,7 @@ def YouCompleteMe_ShowDetailedDiagnostic_MessageFromServer_test(
 
   current_buffer = VimBuffer( 'buffer' )
   with MockVimBuffers( [ current_buffer ], current_buffer ):
-    with patch( 'ycm.client.base_request.JsonFromFuture',
+    with patch( 'ycm.client.base_request._JsonFromFuture',
                 return_value = { 'message': 'some_detailed_diagnostic' } ):
       ycm.ShowDetailedDiagnostic(),
 

python/ycm/youcompleteme.py

@@ -41,8 +41,7 @@ from ycmd.request_wrap import RequestWrap
 from ycm.omni_completer import OmniCompleter
 from ycm import syntax_parse
 from ycm.client.ycmd_keepalive import YcmdKeepalive
-from ycm.client.base_request import ( BaseRequest, BuildRequestData,
+from ycm.client.base_request import BaseRequest, BuildRequestData
-                                      HandleServerException )
 from ycm.client.completer_available_request import SendCompleterAvailableRequest
 from ycm.client.command_request import SendCommandRequest
 from ycm.client.completion_request import CompletionRequest
@@ -228,10 +227,9 @@ class YouCompleteMe( object ):
 
 
   def CheckIfServerIsReady( self ):
-    if not self._server_is_ready_with_cache:
+    if not self._server_is_ready_with_cache and self.IsServerAlive():
-      with HandleServerException( display = False ):
       self._server_is_ready_with_cache = BaseRequest.GetDataFromHandler(
-            'ready' )
+          'ready', display_message = False )
     return self._server_is_ready_with_cache
 
 
@@ -332,10 +330,9 @@ class YouCompleteMe( object ):
 
 
   def GetDefinedSubcommands( self ):
-    with HandleServerException():
+    subcommands = BaseRequest.PostDataToHandler( BuildRequestData(),
-      return BaseRequest.PostDataToHandler( BuildRequestData(),
                                                  'defined_subcommands' )
-    return []
+    return subcommands if subcommands else []
 
 
   def GetCurrentCompletionRequest( self ):
@@ -656,7 +653,6 @@ class YouCompleteMe( object ):
 
 
   def ShowDetailedDiagnostic( self ):
-    with HandleServerException():
     detailed_diagnostic = BaseRequest.PostDataToHandler(
         BuildRequestData(), 'detailed_diagnostic' )