An Anonymous Coward on c.l.py posted a little program with bizarre

behavior, creating many threads very quickly.  A long debugging session
revealed that the Windows implementation of PyThread_start_new_thread()
was choked with "laziness" errors:

1. It checked MS _beginthread() for a failure return, but when that
   happened it returned heap trash as the function result, instead of
   an id of -1 (the proper error-return value).

2. It didn't consider that the Win32 CreateSemaphore() can fail.

3. When creating a great many threads very quickly, it's quite possible
   that any particular bootstrap call can take virtually any amount of
   time to return.  But the code waited for a maximum of 5 seconds, and
   didn't check to see whether the semaphore it was waiting for got
   signaled.  If it in fact timed out, the function could again return
   heap trash as the function result.  This is actually what confused
   the test program, as the heap trash usually turned out to be 0, and
   then multiple threads all got id 0 simultaneously, confusing the
   hell out of threading.py's _active dict (mapping id to thread
   object).  A variety of baffling behaviors followed from that.

WRT #1 and #2, error returns are checked now, and "thread.error: can't
start new thread" gets raised now if a new thread (or new semaphore)
can't be created.  WRT #3, we now wait for the semaphore without a
timeout.

Also removed useless local vrbls, folded long lines, and changed callobj
to a stack auto (it was going thru malloc/free instead, for no discernible
reason).

Bugfix candidate.

diff --git a/Misc/NEWS b/Misc/NEWS
index 479f2e2f7ed4ae20f434683dd679ccf0a953d3c0..2953dc337dca1f82db52bda49558ebe4850adefa 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,6 +10,17 @@ What's New in Python 2.3 release candidate?
 Core and builtins
 -----------------
 
+- The Windows implementation of PyThread_start_new_thread() never
+  checked error returns from Windows functions correctly.  As a result,
+  it could claim to start a new thread even when the Microsoft
+  _beginthread() function failed (due to "too many threads" -- this is
+  on the order of thousands when it happens).  In these cases, the
+  Python exception ::
+
+      thread.error: can't start new thread
+
+  is raised now.
+
 Extension modules
 -----------------
 

diff --git a/Python/thread_nt.h b/Python/thread_nt.h
index 44814c697763b4a8d096f8cbc4d4316e700fd628..7d2290e6b88ac816792f4fb7cebdc7c59da57ec6 100644 (file)
--- a/Python/thread_nt.h
+++ b/Python/thread_nt.h
@@ -148,7 +148,7 @@ static void PyThread__init_thread(void)
 
 typedef struct {
        void (*func)(void*);
-       void *arg;                      
+       void *arg;
        long id;
        HANDLE done;
 } callobj;
@@ -167,35 +167,42 @@ bootstrap(void *call)
        return 0;
 }
 
-long PyThread_start_new_thread(void (*func)(void *), void *arg)
+long
+PyThread_start_new_thread(void (*func)(void *), void *arg)
 {
        unsigned long rv;
-       int success = 0;
-       callobj *obj;
-       int id;
+       callobj obj;
 
-       dprintf(("%ld: PyThread_start_new_thread called\n", PyThread_get_thread_ident()));
+       dprintf(("%ld: PyThread_start_new_thread called\n",
+                PyThread_get_thread_ident()));
        if (!initialized)
                PyThread_init_thread();
 
-       obj = malloc(sizeof(callobj)); 
-       obj->func = func;
-       obj->arg = arg;
-       obj->done = CreateSemaphore(NULL, 0, 1, NULL);
-
-       rv = _beginthread(bootstrap, 0, obj); /* use default stack size */
- 
-       if (rv != (unsigned long)-1) {
-               success = 1;
-               dprintf(("%ld: PyThread_start_new_thread succeeded: %p\n", PyThread_get_thread_ident(), rv));
+       obj.id = -1;    /* guilty until proved innocent */
+       obj.func = func;
+       obj.arg = arg;
+       obj.done = CreateSemaphore(NULL, 0, 1, NULL);
+       if (obj.done == NULL)
+               return -1;
+
+       rv = _beginthread(bootstrap, 0, &obj); /* use default stack size */
+       if (rv == (unsigned long)-1) {
+               /* I've seen errno == EAGAIN here, which means "there are
+                * too many threads".
+                */
+               dprintf(("%ld: PyThread_start_new_thread failed: %p errno %d\n",
+                        PyThread_get_thread_ident(), rv, errno));
+               obj.id = -1;
        }
-
-       /* wait for thread to initialize and retrieve id */
-       WaitForSingleObject(obj->done, 5000);  /* maybe INFINITE instead of 5000? */
-       CloseHandle((HANDLE)obj->done);
-       id = obj->id;
-       free(obj);
-       return id;
+       else {
+               dprintf(("%ld: PyThread_start_new_thread succeeded: %p\n",
+                        PyThread_get_thread_ident(), rv));
+               /* wait for thread to initialize, so we can get its id */
+               WaitForSingleObject(obj.done, INFINITE);
+               assert(obj.id != -1);
+       }
+       CloseHandle((HANDLE)obj.done);
+       return obj.id;
 }
 
 /*