Merge lp:~ben-hutchings/ensoft-sextant/render_timeout into lp:ensoft-sextant

[Ben Hutchings]

Added a timeout option in the (new) [Server] section of the config file. If a graph request takes longer than this value to complete, it will be cancelled and a timeout error message displayed.

[Robert (rjwills)]

This is a bigger fix. In the description, you should explain how you've tested it.

Also, have you tested to see what happens if the Neo4J query takes ages? Ideally we want to abort the query if that is what is taking the time.

review: Needs Fixing

[lp:~ben-hutchings/ensoft-sextant/render_timeout]

27. By Ben Hutchings on 2014-09-30

Merged from parent - double request fix and audit error message fix

28. By Ben Hutchings on 2014-09-30

Initial refactoring of _render_plot

29. By Ben Hutchings on 2014-10-03

Diff comments - fitted _render_plot into 80 columns

Unmerged revisions

=== modified file 'etc/sextant.conf'
--- etc/sextant.conf	2014-09-04 13:04:53 +0000
+++ etc/sextant.conf	2014-10-01 10:37:14 +0000
@@ -17,3 +17,7 @@
 server_bind = 127.0.0.1
 # Port on which to serve Sextant Web
 #port = 2905
+
+[Server]
+# How long to wait before timing out the render
+render_timeout = 10

=== modified file 'src/sextant/__main__.py'
--- src/sextant/__main__.py	2014-09-30 11:01:00 +0000
+++ src/sextant/__main__.py	2014-10-01 10:37:14 +0000
@@ -61,6 +61,7 @@
     # allowing non-web functionality to work with Python 3.
     if sys.version_info[0] == 2:
         from .web import server
+        server.render_timeout = config.render_timeout
     else:  # twisted won't be available - Python 2 required
         logging.error('Web server must be run on Python 2.')
         return

=== modified file 'src/sextant/environment.py'
--- src/sextant/environment.py	2014-09-03 16:38:00 +0000
+++ src/sextant/environment.py	2014-10-01 10:37:14 +0000
@@ -53,7 +53,7 @@
 
 Config = collections.namedtuple('Config', ['web_port', 'remote_neo4j',
                                            'common_cutoff', 'use_ssh_tunnel',
-                                           'ssh_user'])
+                                           'ssh_user', 'render_timeout'])
 
 
 def load_config(check_here=''):
@@ -109,7 +109,8 @@
                                       'port': '2905',
                                       'common_function_calls': '10',
                                       'use_ssh_tunnel': 'False',
-                                      'ssh_username': ''})
+                                      'ssh_username': '',
+                                      'render_timeout': '15'})
     # It turns out that getint, getboolean etc want the defaults to be strings.
     # When getboolean is called and the default is not a string, and the
     # default is used, it complains that it can't work with non-strings.
@@ -123,6 +124,7 @@
     common_def = 0  # definition of a 'common' node
     use_ssh_tunnel = True
     ssh_user = ''
+    render_timeout = 0;
     try:
         conf.options('Neo4j')
     except ConfigParser.NoSectionError:
@@ -132,6 +134,7 @@
         common_def = conf.getint('Neo4j', 'common_function_calls')
         use_ssh_tunnel = conf.getboolean('Neo4j', 'use_ssh_tunnel')
         ssh_user = conf.get('Neo4j', 'ssh_username')
+
     try:
         global SERVER_BIND
         SERVER_BIND = conf.get('Web', 'server_bind')
@@ -139,7 +142,14 @@
     except ConfigParser.NoSectionError:
         pass
 
+    try:
+        conf.options('Server')
+    except ConfigParser.NoSectionError:
+        pass
+    else:
+        render_timeout = conf.getint('Server', 'render_timeout')
+
     return Config(remote_neo4j=remote_neo4j, web_port=web_port,
                   common_cutoff=common_def, use_ssh_tunnel=use_ssh_tunnel,
-                  ssh_user=ssh_user)
+                  ssh_user=ssh_user, render_timeout=render_timeout)
 

=== modified file 'src/sextant/web/server.py'
--- src/sextant/web/server.py	2014-09-05 12:25:37 +0000
+++ src/sextant/web/server.py	2014-10-01 10:37:14 +0000
@@ -28,6 +28,27 @@
 
 database_url = None  # the URL to access the database instance
 
+
+def setTimeout(deferred, timeout=5.0):
+    # cancel the given deferred instance after timeout seconds
+
+    timeout = reactor.callLater(timeout, deferred.cancel)
+
+    def gotResult(result):
+        # if the deferred returns before the timeout calls, cancel the timeout
+        # and return the result of the deferred
+        if timeout.active():
+            timeout.cancel()
+
+        return result
+
+    return deferred.addBoth(gotResult)
+
+def deferToThreadWithTimeout(timeout, function, *args, **kwargs):
+    result = deferToThread(function, *args, **kwargs)
+    return setTimeout(result, timeout)
+
+
 class Echoer(Resource):
     # designed to take one name argument
 
@@ -78,134 +99,126 @@
 
     @defer.inlineCallbacks
     def _render_plot(self, request):
-        if "program_name" not in request.args:
-            request.setResponseCode(400)
-            request.write("Supply 'program_name' parameter.")
-            request.finish()
-            defer.returnValue(None)
-
-        name = request.args["program_name"][0]
-
-        try:
-            suppress_common = request.args["suppress_common"][0]
-        except KeyError:
-            suppress_common = False
-
-        if suppress_common == 'null' or suppress_common == 'true':
-            suppress_common = True
-        else:
-            suppress_common = False
-
-        try:
-            neo4jconnection = yield deferToThread(self.create_neo4j_connection)
-        except requests.exceptions.ConnectionError:
-            request.setResponseCode(502)  # Bad Gateway
-            request.write("Could not reach Neo4j server at {}".format(database_url))
-            request.finish()
-            defer.returnValue(None)
-            neo4jconnection = None  # to silence the "referenced before assignment" warnings later
-
-        exists = yield deferToThread(self.check_program_exists, neo4jconnection, name)
-        if not exists:
-            request.setResponseCode(404)
-            logging.debug('returning nonexistent')
-            request.write("Name %s not found." % (escape(name)))
-            request.finish()
-            defer.returnValue(None)
-
-        allowed_queries = ("whole_program", "functions_calling", "functions_called_by", "all_call_paths", "shortest_call_path")
-
-        if "query" not in request.args:
-            query = "whole_program"
-        else:
-            query = request.args["query"][0]
-
-        if query not in allowed_queries:
-            # raise 400 Bad Request error
-            request.setResponseCode(400)
-            request.write("Supply 'query' parameter, default is whole_program, allowed %s." % str(allowed_queries))
-            request.finish()
-            defer.returnValue(None)
-
-        if query == 'whole_program':
-            program = yield deferToThread(self.get_whole_program, neo4jconnection, name)
-        elif query == 'functions_calling':
-            if 'func1' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func1' parameter to functions_calling.")
-                request.finish()
-                defer.returnValue(None)
-            func1 = request.args['func1'][0]
-            program = yield deferToThread(self.get_functions_calling, neo4jconnection, name, func1)
-        elif query == 'functions_called_by':
-            if 'func1' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func1' parameter to functions_called_by.")
-                request.finish()
-                defer.returnValue(None)
-            func1 = request.args['func1'][0]
-            program = yield deferToThread(neo4jconnection.get_all_functions_called, name, func1)
-        elif query == 'all_call_paths':
-            if 'func1' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func1' parameter to all_call_paths.")
-                request.finish()
-                defer.returnValue(None)
-            if 'func2' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func2' parameter to call_paths.")
-                request.finish()
-                defer.returnValue(None)
-
-            func1 = request.args['func1'][0]
-            func2 = request.args['func2'][0]
-            program = yield deferToThread(neo4jconnection.get_call_paths, name, func1, func2)
-        elif query == 'shortest_call_path':
-            if 'func1' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func1' parameter to shortest_call_path.")
-                request.finish()
-                defer.returnValue(None)
-            if 'func2' not in request.args:
-                # raise 400 Bad Request error
-                request.setResponseCode(400)
-                request.write("Supply 'func2' parameter to shortest_call_path.")
-                request.finish()
-                defer.returnValue(None)
-
-            func1 = request.args['func1'][0]
-            func2 = request.args['func2'][0]
-            program = yield deferToThread(neo4jconnection.get_shortest_path_between_functions, name, func1, func2)
-
-        else:  # unrecognised query, so we need to raise a Bad Request response
-            request.setResponseCode(400)
-            request.write("Query %s not recognised." % escape(query))
-            request.finish()
-            defer.returnValue(None)
-            program = None  # silences the "referenced before assignment" warnings
-
-        if program is None:
-            request.setResponseCode(404)
-            request.write("At least one of the input functions was not found in program %s." % (escape(name)))
-            request.finish()
-            defer.returnValue(None)
-
-        logging.debug('getting plot')
-        if not program.functions:  # we got an empty program back:  the program is in the Sextant but has no functions
-            request.setResponseCode(204)
-            request.finish()
-            defer.returnValue(None)
-        output = yield deferToThread(self.get_plot, program, suppress_common,
-                                     remove_self_calls=False)
-        request.setHeader("content-type", "image/svg+xml")
-
-        logging.debug('SVG: return')
-        request.write(output)
+        # the items in the args dict are lists - so use .get()[0] to retrieve
+        args = request.args
+
+        response_code = 200 # default to okay
+        response_msg = None # set this in the logic
+
+        #
+        # Get the program name and the database connection, see if the program exists
+        #
+
+        name = args.get('program_name', [None])[0]
+
+        if "program_name" is None:
+            response_code = 400
+            response_msg = "Supply 'program_name' parameter."
+
+        if response_code is 200:
+            try:
+                neo4jconnection = yield deferToThread(self.create_neo4j_connection)
+            except requests.exceptions.ConnectionError:
+                response_code = 502 # Bad Gateway
+                response_msg = "Could not reach Neo4j server at {}".format(database_url)
+                neo4jconnection = None
+
+        if response_code is 200:
+            exists = yield deferToThread(self.check_program_exists, neo4jconnection, name)
+            if not exists:
+                response_code = 404
+                response_msg = "Program {} not found in database.".format(escape(name))
+
+        #
+        # We have a connection and a valid program - now setup the query
+        #
+
+        # We store query info as: 
+        # <query_name>: (<function>, (<known args>), (<kwargs>)
+        # where <known args> are local variables and <kwargs> are the keys we
+        # look for in request.args, both tuples
+        queries = {
+            'whole_program':       (
+                                    self.get_whole_program,
+                                    (neo4jconnection, name), 
+                                    ()
+                                   ),
+            'functions_calling':   (
+                                    self.get_functions_calling,
+                                    (neo4jconnection, name), 
+                                    ('func1',)
+                                   ),
+            'functions_called_by': (
+                                    neo4jconnection.get_all_functions_called,
+                                    (name,), 
+                                    ('func1',)
+                                   ),
+            'all_call_paths':      (
+                                    neo4jconnection.get_call_paths,
+                                    (name,),
+                                    ('func1', 'func2')
+                                   ),
+            'shortest_call_path':  (
+                                    neo4jconnection.get_shortest_path_between_functions,
+                                    (name,),
+                                    ('func1', 'func2')
+                                   )
+        }
+
+        # default to whole_program if query is in request.args but has no value
+        query_name = args.get('query', ['whole_program'])[0]
+
+        # check for an invalid query
+        query = queries.get(query_name, None)
+
+        if query is None:
+            # we do not have a query function for this query name
+            response_code = 400 # Bad request
+            response_msg = "Invalid 'query' parameter, allowed: {}".format(queries.keys())
+
+        # extract any required keyword arguments from request.args
+        if response_code is 200:
+            fn, known_args, kwargs = query
+            
+            # all found args will be strings - use None to indicate a missing argument
+            req_args = tuple(args.get(kwarg, [None])[0] for kwarg in kwargs)
+            missing_args = [kwarg for (kwarg, req_arg) in zip(kwargs, req_args) if req_arg is None]
+
+            if missing_args:
+                # missing a kwarg from request.args
+                response_code = 400 # Bad request
+                response_msg = "Missing arguments {} to {}.".format(', '.join(missing_args), name)
+
+        # if we are okay here we have a valid query with all required arguments
+        if response_code is 200:
+            try:
+                program = yield deferToThreadWithTimeout(render_timeout, fn, *(known_args + req_args))
+            except defer.CancelledError:
+                # the timeout has fired and cancelled the request
+                response_code = 400 # Bad request - took too long
+                response_msg = "The request timed out after {} seconds.".format(render_timeout)
+            
+        if response_code is 200:
+            # we have received a response to our request
+            if program is None:
+                response_code = 404
+                response_msg = ("At least one of the input functions '{}' was not "
+                                "found in program {}.").format(', '.join(req_args), escape(name))
+            elif not program.functions:
+                response_code = 204 # No content
+                response_msg = "The program {} is in the database but has no functions.".format(name)
+
+        if response_code is 200:
+            # True if we get 'null' or 'true' as the string value back, otherwise False
+            suppress_common = args.get('suppress_common', ['false'])[0] in ['null', 'true']
+
+            # we have a non-empty return - render it
+            response_msg = yield deferToThread(self.get_plot, program, 
+                                            suppress_common, remove_self_calls=False)
+            request.setHeader('content-type', 'image/svg+xml')
+
+        request.setResponseCode(response_code)
+        request.write(response_msg)
         request.finish()
 
     def render_GET(self, request):