Client - bogus use of threading.local

Bug #530229 reported by Roger Binns on 2010-03-01
18
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Python Memcached
Medium
Unassigned

Bug Description

Client is derived from threading.local presumably at some misguided sense of working across multiple threads. However this doesn't work if the same Client is used across threads serially.

For instance if I create a client in one thread and then call set_servers and then use the Client in another thread the set_servers call would have had no effect since the servers and buckets are per-thread due to threading.local.

Christopher Grebs (shoxi) wrote :

This does also affect the way it works together with greenlets. Making it not inherit from threading.local gives the user the choice of how the client behaves.

Sean Reifschneider (jafo) wrote :

What is the right way to fix this?

Changed in python-memcached:
status: New → Incomplete
importance: Undecided → Medium
Roger Binns (ubuntu-rogerbinns) wrote :

It depends on what you think set_servers should do. If you think it should have no effect on any other thread then leave the code unchanged.

However that is a nightmare because I monitor the current servers config in only one thread and expect the set_servers call to affect all threads. It would be impossible to monitor in all threads. A fix in this case is to remove the use of threading.local. That does however mean that things will get screwed up if you try to use the same client in different threads concurrently. However that is not a scenario anyone actually expects to work anyway - for example none of the Python standard library clients (http etc) can be safely used concurrently from multiple threads.

An alternate fix is to store the server list centrally so that any calls to set_server do affect all the clients in all the threads, although there are issues as to when they would notice the change. You would then need to make the client be in two pieces - a singleton piece and a per thread piece. A lot more work and not worth the effort.

Sean Reifschneider (jafo) wrote :

I've sent a message to the python list asking for feedback on this.
http://mail.python.org/pipermail/python-list/2010-December/1262569.html

Paul Hummer (rockstar) wrote :

Could someone affected by this bug attach some sample code that shows me how you're using python-memcached with your code?

Roger Binns (ubuntu-rogerbinns) wrote :

It wouldn't work here due to the other pieces it talks to. I altered the code to fix bug 529855 and to remove the inheritance from threading.local. Neither issue could be monkey patched to fix them which is the usual approach I take to third party libraries.

My use case is that I have a long running server process. A thread runs in the background that monitors the config server for changes. On changes it calls set_servers to update the server list. Consequently the threading.local stuff means I could never change the servers for an existing instance.

Separately I do have a monkey patch for the _set method. This is because I need to know which host and port was used as that information is communicated to other components. I also made my version raise exceptions rather than do error codes. It is also a little different in that it is a loop that gets the server, tries it and if that fails marks it dead, and tries again until there are no servers left. (ie I do not depend on any of the allocation/hashing algorithms.)

I have a pool of these client instances and then a function that returns one with context management. ie you use it like this:

    with mymemcached.client() as client:
        client.get(,..)
        client.set(...)

Server threads come and go all the time depending on traffic and load so having one Client per thread would result in a huge number of TCP connections being made and dropped, not to mention the difficulty of updating their config.

INADA Naoki (songofacandy) wrote :

How about mixi-in threading.local?

My patched version is: https://code.launchpad.net/~songofacandy/python-memcached/mixin-threading

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers