Change in vdsm[ovirt-3.4]: threadLocal: Cleanup thread local storage

nsoffer at redhat.com nsoffer at redhat.com
Wed Feb 26 22:50:17 UTC 2014


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 at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/24309
Reviewed-by: Adam Litke <alitke at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
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 at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list