From 0b8adeb324a1044cfeac23dd5d987c32997d0e7f Mon Sep 17 00:00:00 2001 From: Ben Darnell Date: Sun, 13 Nov 2011 20:15:44 -0800 Subject: [PATCH] Thread safety for shared data structures (templates and static file hashes) These data structures were basically safe before thanks to the GIL, but could lead to wasted work in multithreaded environments (such as the new python2.7 app engine) Also fixed a bug that made debug mode not invalidate static files. --- tornado/template.py | 17 +++++++++++---- tornado/web.py | 53 ++++++++++++++++++++++++++++----------------- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/tornado/template.py b/tornado/template.py index f28f40355..fc9563016 100644 --- a/tornado/template.py +++ b/tornado/template.py @@ -177,6 +177,7 @@ import logging import os.path import posixpath import re +import threading from tornado import escape from tornado.util import bytes_type @@ -282,10 +283,17 @@ class BaseLoader(object): self.autoescape = autoescape self.namespace = namespace or {} self.templates = {} + # self.lock protects self.templates. It's a reentrant lock + # because templates may load other templates via `include` or + # `extends`. Note that thanks to the GIL this code would be safe + # even without the lock, but could lead to wasted work as multiple + # threads tried to compile the same template simultaneously. + self.lock = threading.RLock() def reset(self): """Resets the cache of compiled templates.""" - self.templates = {} + with self.lock: + self.templates = {} def resolve_path(self, name, parent_path=None): """Converts a possibly-relative path to absolute (used internally).""" @@ -294,9 +302,10 @@ class BaseLoader(object): def load(self, name, parent_path=None): """Loads a template.""" name = self.resolve_path(name, parent_path=parent_path) - if name not in self.templates: - self.templates[name] = self._create_template(name) - return self.templates[name] + with self.lock: + if name not in self.templates: + self.templates[name] = self._create_template(name) + return self.templates[name] def _create_template(self, name): raise NotImplementedError() diff --git a/tornado/web.py b/tornado/web.py index e87f33054..d9a77ccf6 100644 --- a/tornado/web.py +++ b/tornado/web.py @@ -69,6 +69,7 @@ import os.path import re import stat import sys +import threading import time import tornado import traceback @@ -98,6 +99,9 @@ class RequestHandler(object): """ SUPPORTED_METHODS = ("GET", "HEAD", "POST", "DELETE", "PUT", "OPTIONS") + _template_loaders = {} # {path: template.BaseLoader} + _template_loader_lock = threading.Lock() + def __init__(self, application, request, **kwargs): self.application = application self.request = request @@ -537,12 +541,13 @@ class RequestHandler(object): while frame.f_code.co_filename == web_file: frame = frame.f_back template_path = os.path.dirname(frame.f_code.co_filename) - if not getattr(RequestHandler, "_templates", None): - RequestHandler._templates = {} - if template_path not in RequestHandler._templates: - loader = self.create_template_loader(template_path) - RequestHandler._templates[template_path] = loader - t = RequestHandler._templates[template_path].load(template_name) + with RequestHandler._template_loader_lock: + if template_path not in RequestHandler._template_loaders: + loader = self.create_template_loader(template_path) + RequestHandler._template_loaders[template_path] = loader + else: + loader = RequestHandler._template_loaders[template_path] + t = loader.load(template_name) args = dict( handler=self, request=self.request, @@ -1320,10 +1325,10 @@ class Application(object): # In debug mode, re-compile templates and reload static files on every # request so you don't need to restart to see changes if self.settings.get("debug"): - if getattr(RequestHandler, "_templates", None): - for loader in RequestHandler._templates.values(): + with RequestHandler._template_loader_lock: + for loader in RequestHandler._template_loaders.values(): loader.reset() - RequestHandler._static_hashes = {} + StaticFileHandler.reset() handler._execute(transforms, *args, **kwargs) return handler @@ -1424,11 +1429,17 @@ class StaticFileHandler(RequestHandler): CACHE_MAX_AGE = 86400*365*10 #10 years _static_hashes = {} + _lock = threading.Lock() # protects _static_hashes def initialize(self, path, default_filename=None): self.root = os.path.abspath(path) + os.path.sep self.default_filename = default_filename + @classmethod + def reset(cls): + with cls._lock: + cls._static_hashes = {} + def head(self, path): self.get(path, include_body=False) @@ -1517,19 +1528,21 @@ class StaticFileHandler(RequestHandler): is the static path being requested. The url returned should be relative to the current host. """ - hashes = cls._static_hashes abs_path = os.path.join(settings["static_path"], path) - if abs_path not in hashes: - try: - f = open(abs_path, "rb") - hashes[abs_path] = hashlib.md5(f.read()).hexdigest() - f.close() - except Exception: - logging.error("Could not open static file %r", path) - hashes[abs_path] = None + with cls._lock: + hashes = cls._static_hashes + if abs_path not in hashes: + try: + f = open(abs_path, "rb") + hashes[abs_path] = hashlib.md5(f.read()).hexdigest() + f.close() + except Exception: + logging.error("Could not open static file %r", path) + hashes[abs_path] = None + hsh = hashes.get(abs_path) static_url_prefix = settings.get('static_url_prefix', '/static/') - if hashes.get(abs_path): - return static_url_prefix + path + "?v=" + hashes[abs_path][:5] + if hsh: + return static_url_prefix + path + "?v=" + hsh[:5] else: return static_url_prefix + path -- 2.47.2