PR#4330: drop custom threadlocal implementation - koji

koji

#4330 drop custom threadlocal implementation

Merged 2 months ago by tkopecek. Opened 2 months ago by mikem.

mikem/koji use-threadlocal into master

drop custom threadlocal implementation

Mike McLean • 2 months ago

ad81bf3

koji/context.py

file modified

+8 -54

		`@@ -17,67 +17,21 @@`
		`# Authors:`
		`# Mike McLean <mikem@redhat.com>`

		`- # This modules provides a thread-safe way of passing`
		`- # request context around in a global way`
		`- # - db connections`
		`- # - request data`
		`- # - auth data`
		`+ # This module provides a threadlocal instance that kojihub uses to store`
		`+ # request context.`
		`+ # In the past, we had a custom ThreadLocal implementation, but now this is`
		`+ # just a thin wrapper around threading.local`

		`from __future__ import absolute_import`

		`- import six`
		`- import six.moves._thread`
		`+ import threading`


		`- class _data(object):`
		`- pass`
		`-`
		`-`
		`- class ThreadLocal(object):`
		`- def __init__(self):`
		`- object.__setattr__(self, '_tdict', {})`
		`-`
		`- # should probably be getattribute, but easier to debug this way`
		`- def __getattr__(self, key):`
		`- id = six.moves._thread.get_ident()`
		`- tdict = object.__getattribute__(self, '_tdict')`
		`- if id not in tdict:`
		`- raise AttributeError(key)`
		`- data = tdict[id]`
		`- return object.__getattribute__(data, key)`
		`-`
		`- def __setattr__(self, key, value):`
		`- id = six.moves._thread.get_ident()`
		`- tdict = object.__getattribute__(self, '_tdict')`
		`- if id not in tdict:`
		`- tdict[id] = _data()`
		`- data = tdict[id]`
		`- return object.__setattr__(data, key, value)`
		`-`
		`- def __delattr__(self, key):`
		`- id = six.moves._thread.get_ident()`
		`- tdict = object.__getattribute__(self, '_tdict')`
		`- if id not in tdict:`
		`- raise AttributeError(key)`
		`- data = tdict[id]`
		`- ret = object.__delattr__(data, key)`
		`- if len(data.__dict__) == 0:`
		`- del tdict[id]`
		`- return ret`
		`-`
		`- def __str__(self):`
		`- id = six.moves._thread.get_ident()`
		`- tdict = object.__getattribute__(self, '_tdict')`
		`- return "(current thread: %s) {" % id + \`
		`- ", ".join(["%s : %s" % (k, v.__dict__) for (k, v) in six.iteritems(tdict)]) + \`
		`- "}"`
		`+ class ThreadLocal(threading.local):`
		`+ """A small compatibility wrapper around threading.local"""`

		`def _threadclear(self):`
		`- id = six.moves._thread.get_ident()`
		`- tdict = object.__getattribute__(self, '_tdict')`
		`- if id not in tdict:`
		`- return`
		`- del tdict[id]`
		`+ self.__dict__.clear()`


		`context = ThreadLocal()`

kojihub/db.py

file modified

+2 -4

		`@@ -36,6 +36,7 @@`
		`# del psycopg2.extensions.string_types[1266]`
		`import re`
		`import sys`
		`+ import threading`
		`import time`
		`import traceback`

		`@@ -56,10 +57,7 @@`
		`# Apache forks a new worker, and that connection`
		`# will be used to service all requests handled`
		`# by that worker.`
		`- # This probably doesn't need to be a ThreadLocal`
		`- # since Apache is not using threading,`
		`- # but play it safe anyway.`
		`- _DBconn = koji.context.ThreadLocal()`
		`+ _DBconn = threading.local()`

		`logger = logging.getLogger('koji.db')`

mikem commented 2 months ago

The ThreadLocal class in koji.context doesn't really offer anything over threading.local. Better to just use the standard python implementation.

mikem commented 2 months ago

The only reason this class exists is that it predates the threading library (added in python-2.4), or at least the availability of python-2.4 in RHEL. The code works, so there was never a reason to change it.

However, I discovered a deficiency this week. The koji ThreadLocal class will not purge data for expired threads. If a threaded app uses it and cycles through new threads regularly, then this could leak resources. Python's implementation does not share this flaw.