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

[Ben Hutchings]

Refactoring of the _render_plot method - now has a more obvious flow of logic (i.e. will always get to the bottom of the function rather than dropping out in one of the many if/elses).

It should be more straightforward to add more query functions - they are now stored in a dictionary which can be easily added to without changing any of the logic.

Refactoring of the _render_plot method - now has a more obvious flow of logic (i.e. will always get to the bottom of the function rather than dropping out in one of the many if/elses).

It should be more straightforward to add more query functions - they are now stored in a dictionary which can be easily added to without changing any of the logic.

The bulk of the new code is in a block at the bottom of the diff - the whole function has been re-written.

Timing out the neo4j query has proved difficult - it apparently needs a header to
be set on the http request with 'max-execution-time': <milliseconds>
(http://www.markhneedham.com/blog/2013/10/17/neo4j-setting-query-timeout/),
but attempts to modify the neo4j restclient library to allow this have been thus
far unsuccessful.

[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

[Robert (rjwills)]

Just some small stylistic improvements.

review: Needs Fixing

[Robert (rjwills)]

review: Approve

=== modified file 'etc/sextant.conf'
--- etc/sextant.conf	2014-09-04 13:04:53 +0000
+++ etc/sextant.conf	2014-10-03 11:50:56 +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 = 15
 
=== modified file 'src/sextant/__main__.py'
--- src/sextant/__main__.py	2014-09-30 11:01:00 +0000
+++ src/sextant/__main__.py	2014-10-03 11:50:56 +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-03 11:50:56 +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-03 11:50:56 +0000
@@ -28,6 +28,32 @@
 
 database_url = None  # the URL to access the database instance
 
+RESPONSE_CODE_OK = 200
+RESPONSE_CODE_BAD_REQUEST = 400
+RESPONSE_CODE_NOT_FOUND = 404
+RESPONSE_CODE_BAD_GATEWAY = 502
+RESPONSE_CODE_NO_CONTENT = 204
+
+def set_timeout(deferred, timeout=5.0):
+    # cancel the given deferred instance after timeout seconds
+
+    timeout = reactor.callLater(timeout, deferred.cancel)
+
+    def got_result(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(got_result)
+
+def defer_to_thread_with_timeout(timeout, function, *args, **kwargs):
+    result = deferToThread(function, *args, **kwargs)
+    return set_timeout(result, timeout)
+
+
 class Echoer(Resource):
     # designed to take one name argument
 
@@ -78,134 +104,139 @@
 
     @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
+
+        res_code = RESPONSE_CODE_OK 
+        res_msg = None # set this in the logic
+
+        #
+        # Get program name and database connection, check if program exists
+        #
+
+        name = args.get('program_name', [None])[0]
+
+        if "program_name" is None:
+            res_code = RESPONSE_CODE_BAD_REQUEST
+            res_msg = "Supply 'program_name' parameter."
+
+        if res_code is RESPONSE_CODE_OK:
+            try:
+                conn = yield deferToThread(self.create_neo4j_connection)
+            except requests.exceptions.ConnectionError:
+                res_code = RESPONSE_CODE_BAD_GATEWAY 
+                res_fmt = "Could not reach Neo4j server at {}"
+                res_msg = res_fmt.format(database_url)
+                conn = None
+
+        if res_code is RESPONSE_CODE_OK:
+            exists = yield deferToThread(self.check_program_exists, conn, name)
+            if not exists:
+                res_code = RESPONSE_CODE_NOT_FOUND
+                res_fmt = "Program {} not found in database."
+                res_msg = res_fmt.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,
+                (conn, name), 
+                ()
+            ),
+            'functions_calling': (
+                self.get_functions_calling,
+                (conn, name), 
+                ('func1',)
+            ),
+            'functions_called_by': (
+                conn.get_all_functions_called,
+                (name,), 
+                ('func1',)
+            ),
+            'all_call_paths': (
+                conn.get_call_paths,
+                (name,),
+                ('func1', 'func2')
+            ),
+            'shortest_call_path': (
+                conn.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
+            res_code = RESPONSE_CODE_BAD_REQUEST
+            res_fmt = "Invalid 'query' parameter, allowed: {}"
+            res_msg = res_fmt.format(queries.keys())
+
+        # extract any required keyword arguments from request.args
+        if res_code is RESPONSE_CODE_OK:
+            fn, known_args, kwargs = query
+            
+            # all args will be strings - use None to indicate 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
+                res_code = RESPONSE_CODE_BAD_REQUEST 
+                res_fmt = "Missing arguments {} to {}."
+                res_msg = res_fmt.format(', '.join(missing_args), name)
+
+        # if we are okay here we have a valid query with all required arguments
+        if res_code is RESPONSE_CODE_OK:
+            try:
+                all_args = known_args + req_args
+                program = yield defer_to_thread_with_timeout(render_timeout, fn,
+                                                             *all_args)
+            except defer.CancelledError:
+                # the timeout has fired and cancelled the request
+                res_code = RESPONSE_CODE_BAD_REQUEST
+                res_fmt = "The request timed out after {} seconds."
+                res_msg = res_fmt.format(render_timeout)
+            
+        if res_code is RESPONSE_CODE_OK:
+            # we have received a response to our request
+            if program is None:
+                res_code = RESPONSE_CODE_NOT_FOUND
+                res_fmt = ("At least one of the input functions '{}' was not "
+                           "found in program {}.")
+                res_msg = res_fmt.format(', '.join(req_args), escape(name))
+            elif not program.functions:
+                res_code = RESPONSE_CODE_NO_CONTENT 
+                res_fmt = ("The program {} is in the database but has no "
+                           "functions.")
+                res_msg = res_fmt.format(name)
+
+        if res_code is RESPONSE_CODE_OK:
+            # True if 'null' or 'true', otherwise False
+            suppress_common_arg = args.get('suppress_common', ['false'])[0]
+            suppress_common = suppress_common_arg in ('null', 'true')
+
+            # we have a non-empty return - render it
+            res_msg = yield deferToThread(self.get_plot, program, 
+                                          suppress_common, 
+                                          remove_self_calls=False)
+            request.setHeader('content-type', 'image/svg+xml')
+
+        request.setResponseCode(res_code)
+        request.write(res_msg)
         request.finish()
 
     def render_GET(self, request):