classification
Title: race condition in threading._newname()
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.3, Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: open Resolution: accepted
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Peter.Saveliev, amaury.forgeotdarc, pitrou, rhettinger
Priority: normal Keywords: patch

Created on 2011-04-18 11:29 by Peter.Saveliev, last changed 2011-08-13 07:52 by rhettinger.

Files
File name Uploaded Description Edit
thread_test.py Peter.Saveliev, 2011-04-18 11:29 simple test python script that illustrates _newname() race
newname_race_fix.patch Peter.Saveliev, 2011-05-03 08:05 race condition fix (itertools, w/o locking)
Messages (7)
msg133963 - (view) Author: Peter Saveliev (Peter.Saveliev) Date: 2011-04-18 11:29
The _newname() function has no locking.

It is called from the new thread constructor. Such constructor is executed within parent thread. So, when several threads create new threads simultaneously, there can be race condition, leading to the same name for two (or even more) threads.

8<--------------------------------

>>> import threading
>>> import dis
>>> dis.dis(threading._newname)
403           0 LOAD_GLOBAL              0 (_counter)
              3 LOAD_CONST               1 (1)
              6 BINARY_ADD         
              7 STORE_GLOBAL             0 (_counter)

404          10 LOAD_FAST                0 (template)
             13 LOAD_GLOBAL              0 (_counter)
             16 BINARY_MODULO      
             17 RETURN_VALUE       
>>> 
8<--------------------------------

The race condition can be achieved between BINARY_ADD and STORE_GLOBAL. Several threads can do BINARY_ADD before the first one will do STORE_GLOBAL. All racing threads will get the same name.

8<--------------------------------
$ for i in `seq 0 100`; do python thread_test.py |\
 awk -F Thread- '{print $2}' |\
 grep -vE '^$' |\
 sort |\
 uniq -c -d; done
      2 35
      2 12
      ...
8<--------------------------------

As you see, there are cases when several threads can get same name.

Proposals: use thread-safe increment counter (with atomic get-increment) like itertools.counter() (that does not release GIL)
msg135004 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-02 19:59
Do you want to provide a patch?
msg135019 - (view) Author: Peter Saveliev (Peter.Saveliev) Date: 2011-05-03 08:05
Ok, patch attached.

Patch made for Python: 2.6
Tested Python versions: 2.6, 2.7
msg141990 - (view) Author: Peter Saveliev (Peter.Saveliev) Date: 2011-08-12 21:00
Any news? I hope, the change is trivial enough…
msg141994 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-08-12 22:52
Are you sure that counter.next() cannot release the GIL?  Remember that any DECREF can trigger the garbage collector and execute arbitrary code...
msg142015 - (view) Author: Peter Saveliev (Peter.Saveliev) Date: 2011-08-13 07:01
counter.next() is a C routine and it is atomic from Python's point of view — if I understand right.

The test shows that original threading.py leads to a (rare) race here, while with counter object there is no race condition.
msg142016 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-08-13 07:52
I think the patch is correct.

FWIW, my style is to prebind the next method, making the counter completely self-contained (like a closure):

+_counter = itertools.count().next
 def _newname(template="Thread-%d"):
     global _counter
-    _counter = _counter + 1
-    return template % _counter
+    return template % _counter()
History
Date User Action Args
2011-08-13 07:52:09rhettingersetnosy: + rhettinger
messages: + msg142016

assignee: rhettinger
resolution: accepted
2011-08-13 07:01:36Peter.Savelievsetmessages: + msg142015
2011-08-12 22:52:14amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg141994
2011-08-12 21:00:24Peter.Savelievsetmessages: + msg141990
2011-05-03 08:05:51Peter.Savelievsetfiles: + newname_race_fix.patch
keywords: + patch
messages: + msg135019

versions: + Python 2.6
2011-05-02 19:59:34pitrousetversions: + Python 3.1, Python 2.7, Python 3.2, Python 3.3, - Python 2.6
nosy: + pitrou

messages: + msg135004

components: + Library (Lib), - Extension Modules
2011-04-18 11:29:02Peter.Savelievcreate