tax.templatetags.satchmo_tax is bound to explode in your face

Issue #1186 resolved
Patryk Zawadzki created an issue

http://bitbucket.org/chris1610/satchmo/src/tip/satchmo/apps/tax/templatetags/satchmo_tax.py#cl-13

{{{ def _get_taxprocessor(request=None): taxprocessor = get_thread_variable('taxer', None) if not taxprocessor: if request:
user = request.user if user.is_authenticated(): user = user else: user = None else: user = get_current_user()

    taxprocessor = get_tax_processor(user=user)    
    set_thread_variable('taxer', taxprocessor)

return taxprocessor

}}}

This code will do all kinds of bad things as soon as two different users hit the same Python thread (in any time frame, could be days in between).

Caching anything at a global thread level is almost guaranteed to be a bad idea.

Solution: drop the helper and properly call {{{get_tax_processor}}} instead.

Comments (6)

  1. Patryk Zawadzki reporter

    Also: it does not really save any queries as there is no guarantee for subsequent requests to be handled by the same thread (and is usually not the case).

  2. Chris Moffitt repo owner

    I'm thinking it might just make more sense to prefix the key with the user id. That way it would be unique per user and avoid this issue. Thoughts?

  3. Hynek Cernoch

    I think that thread locals are useful (or very useful) for the life time of the request. Django has now advaced configuration of multiple caches. Everything temporary which need longer lifetime should be saved by cache.

    The easiest ugly solution is:

    diff -r 27a62f80ee24 threaded_multihost/middleware.py
    --- a/threaded_multihost/middleware.py	Wed May 18 18:34:35 2011 +0200
    +++ b/threaded_multihost/middleware.py	Mon Jan 02 11:10:19 2012 +0100
    @@ -17,5 +17,6 @@
     
         def process_request(self, request):
             set_thread_variable('request', request)
    +        set_thread_variable('taxer', None)
             set_current_user(request.user)
             
    \ No newline at end of file
    

    A big disadvantage is that the name "taxer" should be not a part of threaded_multihost and that it is not so easy to require upgrade of an external package.

    Better solution is to store "taxer" into "request" to ensure that it will be recycled automatically.

  4. Chris Moffitt repo owner

    I don't like the idea of modifying threaded_multihost for the solution. I think we either make the key unique to users or remove the attempted caching all together. I'm not sure how intensive the tax processor loading process is. This may be a case of premature optimization being the root of all evil :)

  5. Log in to comment