classification
Title: Return code of pthread_* in thread_pthread.h is not used for perror
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Birne94, inada.naoki
Priority: normal Keywords: patch

Created on 2017-03-20 12:08 by Birne94, last changed 2017-03-24 20:10 by inada.naoki. This issue is now closed.

Files
File name Uploaded Description Edit
patch.diff Birne94, 2017-03-20 14:35 review
test.py Birne94, 2017-03-20 20:39
Pull Requests
URL Status Linked Edit
PR 741 merged Birne94, 2017-03-20 15:54
PR 753 merged inada.naoki, 2017-03-21 17:58
Messages (9)
msg289884 - (view) Author: Daniel Birnstiel (Birne94) * Date: 2017-03-20 12:08
Python/thread_pthread.h:145 defines the CHECK_STATUS macro used for printing error messages in case any of the calls fail.

CHECK_STATUS uses perror for formatting an error message, which relies on the global erno being set (see man perror). Since the pthread functions return their status code instead of setting erno (which might not even work in threaded environments), no additional information is displayed. See for example produced by PyThread_release_lock:

pthread_mutex_lock[3]: Undefined error: 0
pthread_cond_signal: Undefined error: 0
pthread_mutex_unlock[3]: Undefined error: 0

The correct solution would be to use strerror(status) in order to show the proper message.
msg289887 - (view) Author: Daniel Birnstiel (Birne94) * Date: 2017-03-20 14:35
I have attached a diff adding a new macro for handling pthread_* status codes. Will submit PR as soon as my CLA is approved.
msg289892 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-03-20 18:00
Could you give us minimum sample Python code to reproduce the error?
msg289904 - (view) Author: Daniel Birnstiel (Birne94) * Date: 2017-03-20 20:39
While you might scold me for the following code, it is just some fiddling with the cpython functions from python side.

Backstory is issue 6721 or related to it. I am working on a multi-process service which uses multiprocessing (or rather the billiard fork). While forking I run into deadlocks as described in the issue when I fork while the io lock is acquired.

In order to find a solution I experimented with the cpython module and tried to release the lock manually after a fork. The code can be found attached (tested with python 3.6/3.7 on OSX Sierra). While it does not really do what it is supposed to do, it shows the problem described in this issue (#29859):

If the internal calls to the pthread_* functions fail, I will only get a generic error message:

pthread_mutex_lock[3]: Undefined error: 0
pthread_cond_signal: Undefined error: 0
pthread_mutex_unlock[3]: Undefined error: 0

In reality the produced error messages should be:

pthread_mutex_lock[3]: Invalid argument
pthread_cond_signal: Invalid argument
pthread_mutex_unlock[3]: Invalid argument

This happens because the pthread_* functions do not set the erno variable as described in the issue description.

Please note that the issue is not only related to my code, but might affect all code using locks. While I suppose there shouldn't be any errors when calling the low level functions, the attached patch/pr will make debugging a lot easier when it actually happens by displaying the correct error message.
msg289908 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-03-21 02:41
I don't know your patch is worth enough or not. (I dislike fprintf(stderr, ...) at least).

But my advice is stop mixing multithreading and fork (except fork+exec).
It's almost impossible.

While Python has GIL, some extension can release GIL and run any C code.
But very vary functions are not fork-safe.  Even malloc and printf are unsafe.

For more information, see "rational" section in 
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_atfork.html
msg289930 - (view) Author: Daniel Birnstiel (Birne94) * Date: 2017-03-21 10:14
While I agree, fprintf it not really nice, I looked through other parts of the python source where information is printed to stderr and fprintf was used there as well, so I fell back to it myself.

% grep -rnw . -e "fprintf(stderr," | wc -l                                                                                                                                                                 178

Using threading and multiprocessing is insane, I know that. Nevertheless the error codes returned by the pthread_* calls are processed incorrectly, so I would consider this a bug worth fixing.
msg289932 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-03-21 10:34
OK, perror() writes to stderr too.  fair enough.
msg290122 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-03-24 20:09
New changeset fe30339534c602af1123e1402e44a1463f91f2e5 by INADA Naoki in branch '3.6':
bpo-29859: Fix error messages from return codes for pthread_* calls (GH-753)
https://github.com/python/cpython/commit/fe30339534c602af1123e1402e44a1463f91f2e5
msg290123 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2017-03-24 20:10
New changeset d7fa6b259e00fca04dbf816bfcf4115fdda14bb7 by INADA Naoki (Daniel Birnstiel) in branch 'master':
bpo-29859: Fix error messages from return codes for pthread_* calls (GH-741)
https://github.com/python/cpython/commit/d7fa6b259e00fca04dbf816bfcf4115fdda14bb7
History
Date User Action Args
2017-03-24 20:10:11inada.naokisetmessages: + msg290123
2017-03-24 20:09:45inada.naokisetmessages: + msg290122
2017-03-21 17:58:10inada.naokisetpull_requests: + pull_request666
2017-03-21 13:13:21Birne94setstatus: open -> closed
resolution: fixed
stage: resolved
2017-03-21 10:34:38inada.naokisetmessages: + msg289932
2017-03-21 10:14:34Birne94setmessages: + msg289930
2017-03-21 02:41:21inada.naokisetmessages: + msg289908
2017-03-20 20:39:45Birne94setfiles: + test.py

messages: + msg289904
2017-03-20 18:00:01inada.naokisetnosy: + inada.naoki
messages: + msg289892
2017-03-20 15:54:15Birne94setpull_requests: + pull_request654
2017-03-20 14:35:31Birne94setfiles: + patch.diff
keywords: + patch
messages: + msg289887

versions: + Python 3.7
2017-03-20 12:08:19Birne94create