search

hugobessa / django-shared-schema-tenants

A lib to help in the creation of shared schema multi tenants applications without suffering
MIT License
20 stars 9 forks source link

Check if better to use threading.local() #19

Open filipeximenes opened 7 years ago

filipeximenes commented 7 years ago

Eg.: https://github.com/filipeximenes/multitenancy/blob/master/multitenancy/threadlocal.py

techdragon commented 6 years ago

While a useful and valid tool, it is generally considered something to be avoided unless necessary. As someone that just started using this library I'd prefer to see thread locals avoided as much as possible. Im not opposed to having helper functions/tools that use it, I just really don't want to see the entire library develop in a direction where it required using thread locals to work. - Just my 2 cents worth of thoughts as a user. 😃

hugobessa commented 6 years ago

@techdragon did you see we're already using threading on this package, right? https://github.com/hugobessa/django-shared-schema-tenants/blob/dev/shared_schema_tenants/middleware.py#L57

Don't you think with threading.local this would be more organized?

techdragon commented 6 years ago

I do agree it makes it more organised. However for the same reasons outlined in the rational section of PEP 567 I think thread local storage based solutions are less than ideal solutions where alternative ones exist. Why it concerns me with this library in particular is that I'm looking down the road a little way to a future where I have my Django View code running in an asynchronous environment like ASGI on top of Daphne or Pulsar, or the model code used by asyncio based task worker tools.

When I commented yesterday I was unaware of PEP 567 so I can now happily point out that because of PEP 567 Python >3.7 there is a better alternative (at least in Python 3.7 and above) specifically for the sort of asynchronous related issues that underly my concerns about relying on thread-local storage.

Case in point (which also provides a handy example) , the Decimal module now uses contextvars instead of threading. The bug report Migrate decimal to use PEP 567 context variables and the associated pull request to the python source code bpo-32630: Use contextvars in the decimal module seem to be good examples of how to do things more safely with respect to asynchronous code safety at least in >3.7.