classification
Title: tp_print slots don't release the GIL
Type: behavior Stage:
Components: Interpreter Core Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: arigo, brett.cannon, gvanrossum
Priority: normal Keywords: patch

Created on 2007-09-14 19:21 by arigo, last changed 2007-09-17 03:29 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
deadlock1.py arigo, 2007-09-14 19:21
release_GIL_around_stdout.diff brett.cannon, 2007-09-15 21:34
GIL_release_tp_print.diff brett.cannon, 2007-09-16 03:37
Messages (10)
msg55913 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-09-14 19:21
The tp_print slots, used by 'print' statements, don't release the GIL
while writing into the C-level file.  This is not only a missing
opportunity, but can cause unexpected deadlocks, as shown in the
attached file.
msg55920 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-09-14 21:37
I quickly handled the GIL for lists and ints in their tp_print functions
and your example still deadlocked.  Don't have time to dig deeper right
now, but what more are you suggesting be done?
msg55926 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2007-09-15 08:30
We need to start from PyFile_WriteString() and PyFile_WriteObject()
and PyObject_Print(), which are what the PRINT_* opcodes use, and make
sure there is no single fprintf() or fputs() that occurs with the GIL
held.  It is not enough to fix a few places that could clearly write a
lot of data, because even if there is just one place left where the GIL
is not released it could be precisely where the OS decides to block,
causing a deadlock.
msg55930 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-09-15 18:59
Plese do submit a patch.

FWIW I think it's solved in Py3k, the tp_print slot is dead (as is any
use of the C stdio library).
msg55931 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-09-15 21:34
Since I already did this once I just did a more thorough job; patch is
attached.

PyObject_WriteString() just calls PyFile_WriteObject() which ends up
using PyObject_Print(), so it is was simple to handle those cases.  I
then released the GIL for strings, lists, and ints.  That is enough to
pass Armin's deadlock test.

If the approach I am taking is OK with people and I can go through
Object/*.c and do the proper thing for all the various object types for
fprintf(), fputs(), fputc() and fwrite() (while skipping all of the
debug output stuff) before committing the changes.
msg55935 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-09-16 02:18
Looks Good, except I think it's a bad idea to release/acquire the GIL
for each character when writing the repr() of a string.  Given that the
string is immutable and its refcount kept alive by the caller I don't
see a reason why you can't just reference the object without holding the
GIL.  (Also you might want to copy the size into a local variable, it
won't change...)
msg55936 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-09-16 02:57
Good point.  I changed it on my machine and it cut that whole section
down to a single release/acquire.

I will go ahead and do this for the other types and then upload another
patch to be reviewed when it's ready.
msg55937 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-09-16 03:37
OK, every PyTypeObject in Objects and Modules has its tp_print release
the GIL.  Once someone OKs the patch I will apply it.
msg55948 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-09-17 03:00
looks good to me :)
msg55949 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-09-17 03:29
Applied in r58176.  And I am glad this is not an issue in Py3K.  =)
History
Date User Action Args
2007-09-17 03:29:26brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg55949
2007-09-17 03:00:02gvanrossumsetassignee: brett.cannon
messages: + msg55948
2007-09-16 03:37:32brett.cannonsetfiles: + GIL_release_tp_print.diff
messages: + msg55937
2007-09-16 02:57:41brett.cannonsetmessages: + msg55936
2007-09-16 02:19:01gvanrossumsetmessages: + msg55935
2007-09-15 21:35:10brett.cannonsetkeywords: + patch
2007-09-15 21:34:56brett.cannonsetversions: + Python 2.6
2007-09-15 21:34:47brett.cannonsetfiles: + release_GIL_around_stdout.diff
messages: + msg55931
2007-09-15 18:59:13gvanrossumsetnosy: + gvanrossum
messages: + msg55930
2007-09-15 08:30:12arigosetmessages: + msg55926
2007-09-14 21:37:55brett.cannonsetnosy: + brett.cannon
messages: + msg55920
2007-09-14 19:21:37arigocreate