From 6ec1b0b4a1df7d9dc658d5bf28ae58e37c605c9a Mon Sep 17 00:00:00 2001 From: micbou Date: Wed, 17 May 2017 11:12:00 +0200 Subject: [PATCH] Refactor server exception handling --- python/ycm/client/base_request.py | 109 +++++++++--------- python/ycm/client/command_request.py | 8 +- .../ycm/client/completer_available_request.py | 8 +- python/ycm/client/completion_request.py | 30 ++--- python/ycm/client/debug_info_request.py | 8 +- python/ycm/client/event_notification.py | 7 +- python/ycm/client/messages_request.py | 19 +-- python/ycm/client/shutdown_request.py | 8 +- python/ycm/client/ycmd_keepalive.py | 5 +- python/ycm/omni_completer.py | 9 +- python/ycm/tests/completion_test.py | 32 ++--- python/ycm/tests/event_notification_test.py | 15 +-- python/ycm/tests/youcompleteme_test.py | 6 +- python/ycm/youcompleteme.py | 28 ++--- 14 files changed, 140 insertions(+), 152 deletions(-) diff --git a/python/ycm/client/base_request.py b/python/ycm/client/base_request.py index 03546db2..05949d84 100644 --- a/python/ycm/client/base_request.py +++ b/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 ): - return JsonFromFuture( BaseRequest._TalkToHandlerAsync( '', - handler, - 'GET', - timeout ) ) + def GetDataFromHandler( handler, + timeout = _READ_TIMEOUT_SEC, + display_message = True, + truncate_message = False ): + 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 ): - return JsonFromFuture( BaseRequest.PostDataToHandlerAsync( data, - handler, - timeout ) ) + def PostDataToHandler( data, + handler, + 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 ): diff --git a/python/ycm/client/command_request.py b/python/ycm/client/command_request.py index e2aa2229..4a5e16d1 100644 --- a/python/ycm/client/command_request.py +++ b/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, - HandleServerException ) +from ycm.client.base_request import BaseRequest, BuildRequestData from ycm import vimsupport from ycmd.utils import ToUnicode @@ -53,9 +52,8 @@ class CommandRequest( BaseRequest ): 'completer_target': self._completer_target, 'command_arguments': self._arguments } ) - with HandleServerException(): - self._response = self.PostDataToHandler( request_data, - 'run_completer_command' ) + self._response = self.PostDataToHandler( request_data, + 'run_completer_command' ) def Response( self ): diff --git a/python/ycm/client/completer_available_request.py b/python/ycm/client/completer_available_request.py index d9b11e25..9ee9dd3f 100644 --- a/python/ycm/client/completer_available_request.py +++ b/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, - HandleServerException ) +from ycm.client.base_request import BaseRequest, BuildRequestData class CompleterAvailableRequest( BaseRequest ): @@ -36,9 +35,8 @@ 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' ) + self._response = self.PostDataToHandler( request_data, + 'semantic_completion_available' ) def Response( self ): diff --git a/python/ycm/client/completion_request.py b/python/ycm/client/completion_request.py index 6038ed66..3eb62282 100644 --- a/python/ycm/client/completion_request.py +++ b/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, - HandleServerException, +from ycm.client.base_request import ( BaseRequest, DisplayServerException, 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 ): - self._response = JsonFromFuture( self._response_future ) + response = self.HandleFuture( 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', [] ) - for e in errors: - with HandleServerException( truncate = True ): - raise MakeServerException( e ) + # Vim may not be able to convert the 'errors' entry to its internal format + # so we remove it from the response. + errors = response.pop( 'errors', [] ) + for e in errors: + exception = MakeServerException( e ) + _logger.error( exception ) + DisplayServerException( exception, truncate_message = True ) - return self._response + return response def Response( self ): diff --git a/python/ycm/client/debug_info_request.py b/python/ycm/client/debug_info_request.py index 3e6c5854..ef1d8688 100644 --- a/python/ycm/client/debug_info_request.py +++ b/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, - HandleServerException ) +from ycm.client.base_request import BaseRequest, BuildRequestData 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, 'debug_info' ) + self._response = self.PostDataToHandler( request_data, + 'debug_info', + display_message = False ) def Response( self ): diff --git a/python/ycm/client/event_notification.py b/python/ycm/client/event_notification.py index 9ccafc23..bdbe07e3 100644 --- a/python/ycm/client/event_notification.py +++ b/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, - JsonFromFuture, HandleServerException ) +from ycm.client.base_request import BaseRequest, BuildRequestData 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 = JsonFromFuture( self._response_future ) + self._cached_response = self.HandleFuture( self._response_future, + truncate_message = True ) return self._cached_response if self._cached_response else [] diff --git a/python/ycm/client/messages_request.py b/python/ycm/client/messages_request.py index 12c87ad8..07c9159c 100644 --- a/python/ycm/client/messages_request.py +++ b/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,13 +63,16 @@ class MessagesPoll( BaseRequest ): # Nothing yet... return True - with HandleServerException( display = False ): - response = JsonFromFuture( self._response_future ) + response = self.HandleFuture( 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: - self._SendRequest() - return True + poll_again = _HandlePollResponse( response, diagnostics_handler ) + if poll_again: + self._SendRequest() + return True return False diff --git a/python/ycm/client/shutdown_request.py b/python/ycm/client/shutdown_request.py index 8bef0235..f283f39f 100644 --- a/python/ycm/client/shutdown_request.py +++ b/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( {}, 'shutdown', TIMEOUT_SECONDS ) + self.PostDataToHandler( {}, + 'shutdown', + TIMEOUT_SECONDS, + display_message = False ) def SendShutdownRequest(): diff --git a/python/ycm/client/ycmd_keepalive.py b/python/ycm/client/ycmd_keepalive.py index a36d6998..3e148505 100644 --- a/python/ycm/client/ycmd_keepalive.py +++ b/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' ) + BaseRequest.GetDataFromHandler( 'healthy', display_message = False ) diff --git a/python/ycm/omni_completer.py b/python/ycm/omni_completer.py index 25ec3459..9296fb9c 100644 --- a/python/ycm/omni_completer.py +++ b/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(): - return BaseRequest.PostDataToHandler( request_data, - 'filter_and_sort_candidates' ) - return candidates + response = BaseRequest.PostDataToHandler( request_data, + 'filter_and_sort_candidates' ) + return response if response is not None else candidates diff --git a/python/ycm/tests/completion_test.py b/python/ycm/tests/completion_test.py index 831e034b..cb15d2dc 100644 --- a/python/ycm/tests/completion_test.py +++ b/python/ycm/tests/completion_test.py @@ -42,22 +42,19 @@ 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.completion_request.CompletionRequest.' - 'PostDataToHandlerAsync', - return_value = MagicMock( return_value=True ) ): + 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) - # which calls the supplied callback method. - # - # 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 + # We set up a fake response. + with patch( 'ycm.client.base_request._JsonFromFuture', + side_effect = response_method ): + yield @YouCompleteMeInstance() @@ -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, - post_vim_message, - logger ): +def SendCompletionRequest_ResponseContainingError_test( ycm, post_vim_message ): 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 ) ] ) diff --git a/python/ycm/tests/event_notification_test.py b/python/ycm/tests/event_notification_test.py index 1a2d882c..096b547f 100644 --- a/python/ycm/tests/event_notification_test.py +++ b/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 - # calls the supplied callback method. Generally this callback just raises an - # apropriate exception, otherwise it would have to return a mock future - # object. - # - # 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', + # We set up a fake a Response (as called by EventNotification.Response) + # which calls the supplied callback method. Generally this callback just + # raises an apropriate exception, otherwise it would have to return a mock + # future object. + with patch( 'ycm.client.base_request._JsonFromFuture', side_effect = response_method ): # Filetype available information comes from the server, so rather than diff --git a/python/ycm/tests/youcompleteme_test.py b/python/ycm/tests/youcompleteme_test.py index 8d8feeb5..39a6e122 100644 --- a/python/ycm/tests/youcompleteme_test.py +++ b/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(), diff --git a/python/ycm/youcompleteme.py b/python/ycm/youcompleteme.py index ba23719f..250a492d 100644 --- a/python/ycm/youcompleteme.py +++ b/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, - HandleServerException ) +from ycm.client.base_request import BaseRequest, BuildRequestData 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: - with HandleServerException( display = False ): - self._server_is_ready_with_cache = BaseRequest.GetDataFromHandler( - 'ready' ) + if not self._server_is_ready_with_cache and self.IsServerAlive(): + self._server_is_ready_with_cache = BaseRequest.GetDataFromHandler( + 'ready', display_message = False ) return self._server_is_ready_with_cache @@ -332,10 +330,9 @@ class YouCompleteMe( object ): def GetDefinedSubcommands( self ): - with HandleServerException(): - return BaseRequest.PostDataToHandler( BuildRequestData(), - 'defined_subcommands' ) - return [] + subcommands = BaseRequest.PostDataToHandler( BuildRequestData(), + 'defined_subcommands' ) + return subcommands if subcommands else [] def GetCurrentCompletionRequest( self ): @@ -656,13 +653,12 @@ class YouCompleteMe( object ): def ShowDetailedDiagnostic( self ): - with HandleServerException(): - detailed_diagnostic = BaseRequest.PostDataToHandler( - BuildRequestData(), 'detailed_diagnostic' ) + detailed_diagnostic = BaseRequest.PostDataToHandler( + BuildRequestData(), 'detailed_diagnostic' ) - if 'message' in detailed_diagnostic: - vimsupport.PostVimMessage( detailed_diagnostic[ 'message' ], - warning = False ) + if 'message' in detailed_diagnostic: + vimsupport.PostVimMessage( detailed_diagnostic[ 'message' ], + warning = False ) def ForceCompileAndDiagnostics( self ):