| msg55913 - (view) |
Author: Armin Rigo (arigo) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2007-09-17 03:00 |
| looks good to me :) |
|
|
| msg55949 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2007-09-17 03:29 |
| Applied in r58176. And I am glad this is not an issue in Py3K. =) |
|
|