Possible GIL/threading issue involving subprocess and PyMem_MALLOC...
This seems odd to me so I wanted to see what others think. The unit test Lib/unittest/test/test_runner.py:Test_TextRunner.test_warnings will eventually hit subprocess.Popen._communicate. The `mswindows` implementation of this method relies on threads to buffer stdin/stdout. That'll eventually result in PyOs_StdioReadline being called without the GIL being held. PyOs_StdioReadline calls PyMem_MALLOC, PyMem_FREE and possibly PyMem_REALLOC. On a debug build, these macros are redirected to their _PyMem_Debug* counterparts. The call hierarchy for _PyMem_DebugMalloc looks like this: void * _PyMem_DebugMalloc(size_t nbytes) { return _PyObject_DebugMallocApi(_PYMALLOC_MEM_ID, nbytes); } /* generic debug memory api, with an "id" to identify the API in use */ void * _PyObject_DebugMallocApi(char id, size_t nbytes) { uchar *p; /* base address of malloc'ed block */ uchar *tail; /* p + 2*SST + nbytes == pointer to tail pad bytes */ size_t total; /* nbytes + 4*SST */ bumpserialno(); ------------^^^^^^^^^^^^^^^ total = nbytes + 4*SST; if (total < nbytes) /* overflow: can't represent total as a size_t */ return NULL; p = (uchar *)PyObject_Malloc(total); -------------------------^^^^^^^^^^^^^^^^^^^^^^^ if (p == NULL) return NULL; <snip> Both bumpserialno() and PyObject_Malloc affect global state. The latter also has a bunch of LOCK() and UNLOCK() statements, but these end up being no-ops: /* * Python's threads are serialized, * so object malloc locking is disabled. */ #define SIMPLELOCK_DECL(lock) /* simple lock declaration */ #define SIMPLELOCK_INIT(lock) /* allocate (if needed) and ... */ #define SIMPLELOCK_FINI(lock) /* free/destroy an existing */ #define SIMPLELOCK_LOCK(lock) /* acquire released lock */ #define SIMPLELOCK_UNLOCK(lock) /* release acquired lock */ ... /* * This malloc lock */ SIMPLELOCK_DECL(_malloc_lock) #define LOCK() SIMPLELOCK_LOCK(_malloc_lock) #define UNLOCK() SIMPLELOCK_UNLOCK(_malloc_lock) #define LOCK_INIT() SIMPLELOCK_INIT(_malloc_lock) #define LOCK_FINI() SIMPLELOCK_FINI(_malloc_lock) The PyObject_Malloc() one concerns me the most, as it affects huge amounts of global state. Also, I just noticed PyOs_StdioReadline() can call PyErr_SetString, which will result in a bunch of other calls that should only be made whilst the GIL is held. So, like I said, this seems like a bit of a head scratcher. Legit issue or am I missing something? Trent.
On Thu, Dec 20, 2012 at 10:43 AM, Trent Nelson <trent@snakebite.org> wrote:
This seems odd to me so I wanted to see what others think. The unit test Lib/unittest/test/test_runner.py:Test_TextRunner.test_warnings will eventually hit subprocess.Popen._communicate.
The `mswindows` implementation of this method relies on threads to buffer stdin/stdout. That'll eventually result in PyOs_StdioReadline being called without the GIL being held. PyOs_StdioReadline calls PyMem_MALLOC, PyMem_FREE and possibly PyMem_REALLOC.
Those threads are implemented in Python so how would the GIL ever not be held? -gps
On a debug build, these macros are redirected to their _PyMem_Debug* counterparts. The call hierarchy for _PyMem_DebugMalloc looks like this:
void * _PyMem_DebugMalloc(size_t nbytes) { return _PyObject_DebugMallocApi(_PYMALLOC_MEM_ID, nbytes); }
/* generic debug memory api, with an "id" to identify the API in use */ void * _PyObject_DebugMallocApi(char id, size_t nbytes) { uchar *p; /* base address of malloc'ed block */ uchar *tail; /* p + 2*SST + nbytes == pointer to tail pad bytes */ size_t total; /* nbytes + 4*SST */
bumpserialno(); ------------^^^^^^^^^^^^^^^
total = nbytes + 4*SST; if (total < nbytes) /* overflow: can't represent total as a size_t */ return NULL;
p = (uchar *)PyObject_Malloc(total); -------------------------^^^^^^^^^^^^^^^^^^^^^^^ if (p == NULL) return NULL;
<snip>
Both bumpserialno() and PyObject_Malloc affect global state. The latter also has a bunch of LOCK() and UNLOCK() statements, but these end up being no-ops:
/* * Python's threads are serialized, * so object malloc locking is disabled. */ #define SIMPLELOCK_DECL(lock) /* simple lock declaration */ #define SIMPLELOCK_INIT(lock) /* allocate (if needed) and ... */ #define SIMPLELOCK_FINI(lock) /* free/destroy an existing */ #define SIMPLELOCK_LOCK(lock) /* acquire released lock */ #define SIMPLELOCK_UNLOCK(lock) /* release acquired lock */ ... /* * This malloc lock */ SIMPLELOCK_DECL(_malloc_lock) #define LOCK() SIMPLELOCK_LOCK(_malloc_lock) #define UNLOCK() SIMPLELOCK_UNLOCK(_malloc_lock) #define LOCK_INIT() SIMPLELOCK_INIT(_malloc_lock) #define LOCK_FINI() SIMPLELOCK_FINI(_malloc_lock)
The PyObject_Malloc() one concerns me the most, as it affects huge amounts of global state. Also, I just noticed PyOs_StdioReadline() can call PyErr_SetString, which will result in a bunch of other calls that should only be made whilst the GIL is held.
So, like I said, this seems like a bit of a head scratcher. Legit issue or am I missing something?
Trent. _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/greg%40krypto.org
On Thu, Dec 20, 2012 at 05:47:40PM -0800, Gregory P. Smith wrote:
On Thu, Dec 20, 2012 at 10:43 AM, Trent Nelson <trent@snakebite.org> wrote:
This seems odd to me so I wanted to see what others think. The unit test Lib/unittest/test/test_runner.py:Test_TextRunner.test_warnings will eventually hit subprocess.Popen._communicate.
The `mswindows` implementation of this method relies on threads to buffer stdin/stdout. That'll eventually result in PyOs_StdioReadline being called without the GIL being held. PyOs_StdioReadline calls PyMem_MALLOC, PyMem_FREE and possibly PyMem_REALLOC.
Those threads are implemented in Python so how would the GIL ever not be held? -gps
PyOS_Readline drops the GIL prior to calling PyOS_StdioReadline: Py_BEGIN_ALLOW_THREADS --------^^^^^^^^^^^^^^^^^^^^^^ #ifdef WITH_THREAD PyThread_acquire_lock(_PyOS_ReadlineLock, 1); #endif /* This is needed to handle the unlikely case that the * interpreter is in interactive mode *and* stdin/out are not * a tty. This can happen, for example if python is run like * this: python -i < test1.py */ if (!isatty (fileno (sys_stdin)) || !isatty (fileno (sys_stdout))) rv = PyOS_StdioReadline (sys_stdin, sys_stdout, prompt); -----------------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ else rv = (*PyOS_ReadlineFunctionPointer)(sys_stdin, sys_stdout, prompt); Py_END_ALLOW_THREADS Trent.
I ran into this the other day. I had put in hooks in the PyMem_MALLOC to track memory per tasklet, and it crashed in those cases because it was being called without the GIL. My local patch was simply to _not_ release the GIL. Clearly, calling PyMem_MALLOC without the GIL is an API violation. K
-----Original Message----- From: Python-Dev [mailto:python-dev- bounces+kristjan=ccpgames.com@python.org] On Behalf Of Trent Nelson Sent: 21. desember 2012 03:13 To: Gregory P. Smith Cc: Python-Dev Subject: Re: [Python-Dev] Possible GIL/threading issue involving subprocess and PyMem_MALLOC...
On Thu, Dec 20, 2012 at 05:47:40PM -0800, Gregory P. Smith wrote:
On Thu, Dec 20, 2012 at 10:43 AM, Trent Nelson <trent@snakebite.org> wrote:
This seems odd to me so I wanted to see what others think. The unit test Lib/unittest/test/test_runner.py:Test_TextRunner.test_warnings will eventually hit subprocess.Popen._communicate.
The `mswindows` implementation of this method relies on threads to buffer stdin/stdout. That'll eventually result in PyOs_StdioReadline being called without the GIL being held. PyOs_StdioReadline calls PyMem_MALLOC, PyMem_FREE and possibly PyMem_REALLOC.
Those threads are implemented in Python so how would the GIL ever not be held? -gps
PyOS_Readline drops the GIL prior to calling PyOS_StdioReadline:
Py_BEGIN_ALLOW_THREADS --------^^^^^^^^^^^^^^^^^^^^^^ #ifdef WITH_THREAD PyThread_acquire_lock(_PyOS_ReadlineLock, 1); #endif
/* This is needed to handle the unlikely case that the * interpreter is in interactive mode *and* stdin/out are not * a tty. This can happen, for example if python is run like * this: python -i < test1.py */ if (!isatty (fileno (sys_stdin)) || !isatty (fileno (sys_stdout))) rv = PyOS_StdioReadline (sys_stdin, sys_stdout, prompt); ---------------- -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ else rv = (*PyOS_ReadlineFunctionPointer)(sys_stdin, sys_stdout, prompt); Py_END_ALLOW_THREADS
Trent. _______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python- dev/kristjan%40ccpgames.com
Le Fri, 21 Dec 2012 09:31:44 +0000, Kristján Valur Jónsson <kristjan@ccpgames.com> a écrit :
I ran into this the other day. I had put in hooks in the PyMem_MALLOC to track memory per tasklet, and it crashed in those cases because it was being called without the GIL. My local patch was simply to _not_ release the GIL. Clearly, calling PyMem_MALLOC without the GIL is an API violation.
Indeed, this deserves fixing. (it would be better to still release the GIL around the low-level I/O call, of course) Thanks Trent for finding this! Antoine.
On Fri, Dec 21, 2012 at 01:43:11AM -0800, Antoine Pitrou wrote:
Le Fri, 21 Dec 2012 09:31:44 +0000, Kristján Valur Jónsson <kristjan@ccpgames.com> a écrit :
I ran into this the other day. I had put in hooks in the PyMem_MALLOC to track memory per tasklet, and it crashed in those cases because it was being called without the GIL. My local patch was simply to _not_ release the GIL. Clearly, calling PyMem_MALLOC without the GIL is an API violation.
Indeed, this deserves fixing. (it would be better to still release the GIL around the low-level I/O call, of course)
Created http://bugs.python.org/issue16742 to capture the issue for now. I want to make some more progress on the parallel stuff first so if somebody wants to tackle it in the meantime, be my guest.
Thanks Trent for finding this!
Unexpected (but handy) side-effect of the parallel context work :-) (I wonder if that's the only thread-safe issue in our code base...)
Antoine.
Trent.
participants (4)
-
Antoine Pitrou -
Gregory P. Smith -
Kristján Valur Jónsson -
Trent Nelson