Hello Adam Litke, Dan Kenigsberg, I'd like you to do a code review. Please visit http://gerrit.ovirt.org/25123 to review the following change. Change subject: threadLocal: Cleanup thread local storage ...................................................................... threadLocal: Cleanup thread local storage We keep some data in thread local storage, but never clean up. This can lead to using stale data when handling many requests in the same tread, or to temporary leak, when a long running thread holds a reference to a finished task, preventing garbage collection of anything referenced from the task. This patch ensures that anything we set thread local storage is cleaned up when not needed. Change-Id: I97b110db1c5f94e8561b4ba15a3c27b43043ed6f Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; Reviewed-on: http://gerrit.ovirt.org/24309 Reviewed-by: Adam Litke <alitke(a)redhat.com&gt; Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt; --- M vdsm/BindingXMLRPC.py M vdsm/storage/dispatcher.py M vdsm/storage/storage_mailbox.py M vdsm/storage/task.py 4 files changed, 25 insertions(+), 6 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/25123/1 diff --git a/vdsm/BindingXMLRPC.py b/vdsm/BindingXMLRPC.py index 9b56ea1..bafc7c7 100644 --- a/vdsm/BindingXMLRPC.py +++ b/vdsm/BindingXMLRPC.py @@ -212,6 +212,12 @@ threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID) return r + def finish(self): + basehandler.finish(self) + threadLocal.client = None + threadLocal.server = None + threadLocal.flowID = None + if self.enableSSL: KEYFILE, CERTFILE, CACERT = self._getKeyCertFilenames() server = SecureXMLRPCServer.SecureThreadedXMLRPCServer( diff --git a/vdsm/storage/dispatcher.py b/vdsm/storage/dispatcher.py index cbeb23c..da838e8 100644 --- a/vdsm/storage/dispatcher.py +++ b/vdsm/storage/dispatcher.py @@ -23,7 +23,6 @@ import task import storage_exception as se -from threadLocal import vars _EXPORTED_ATTRIBUTE = "__dispatcher_exported__" @@ -56,7 +55,6 @@ def run(self, *args, **kwargs): try: ctask = task.Task(id=None, name=self.name) - vars.task = ctask try: response = self.STATUS_OK.copy() result = ctask.prepare(self.func, *args, **kwargs) diff --git a/vdsm/storage/storage_mailbox.py b/vdsm/storage/storage_mailbox.py index d56a2d7..4f54066 100644 --- a/vdsm/storage/storage_mailbox.py +++ b/vdsm/storage/storage_mailbox.py @@ -33,7 +33,6 @@ import sd import misc import task -from threadLocal import vars from threadPool import ThreadPool from storage_exception import InvalidParameterException from vdsm import constants @@ -77,7 +76,6 @@ cmd = args args = None ctask = task.Task(id=None, name=cmd) - vars.task = ctask ctask.prepare(cmd, *args) diff --git a/vdsm/storage/task.py b/vdsm/storage/task.py index 4eff5c1..88ab5fa 100644 --- a/vdsm/storage/task.py +++ b/vdsm/storage/task.py @@ -49,6 +49,7 @@ import logging import threading import types +from functools import wraps import storage_exception as se import uuid @@ -88,6 +89,21 @@ def _eq_decode(s): return s.replace(KEY_SEPARATOR_ENCODED, KEY_SEPARATOR) + + +def threadlocal_task(m): + """ + Decorator that set the task object in thread local storage task attribute + while the decorated method is running. + """ + @wraps(m) + def wrapper(self, *a, **kw): + vars.task = self + try: + return m(self, *a, **kw) + finally: + vars.task = None + return wrapper class State: @@ -1133,6 +1149,7 @@ t._load(store, ext) return t + @threadlocal_task def prepare(self, func, *args, **kwargs): message = self.error try: @@ -1172,9 +1189,9 @@ finally: self._decref() + @threadlocal_task def commit(self, args=None): self.log.debug("committing task: %s", self.id) - vars.task = self try: self._incref() except se.TaskAborted: @@ -1210,13 +1227,13 @@ finally: self._decref(force) + @threadlocal_task def recover(self, args=None): ''' Do not call this function while the task is actually running. this method should only be used to recover tasks state after (vdsmd) restart. ''' self.log.debug('(recover): recovering: state %s', self.state) - vars.task = self try: self._incref(force=True) except se.TaskAborted: -- To view, visit http://gerrit.ovirt.org/25123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I97b110db1c5f94e8561b4ba15a3c27b43043ed6f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.4 Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt; Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt; Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;