interruption of locks by signals not guaranteed when locks are implemented using POSIX condition variables; add sys.thread_info

[sable]

BPO	11223
Nosy	@gpshead, @db3l, @pitrou, @vstinner, @benjaminp, @rnk
Files	test_threadsignals.patch pthread_cond_timedwait_signal.c threading_info.patch threading_info-2.patch threading_info-3.patch sys_thread_info.patch sys_thread_info-2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2011-04-20.08:30:34.676>
created_at = <Date 2011-02-16.14:25:35.643>
labels = ['interpreter-core', 'type-bug', 'tests']
title = 'interruption of locks by signals not guaranteed when locks are implemented using POSIX condition variables; add sys.thread_info'
updated_at = <Date 2011-06-23.12:22:50.509>
user = 'https://bugs.python.org/sable'

bugs.python.org fields:

activity = <Date 2011-06-23.12:22:50.509>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2011-04-20.08:30:34.676>
closer = 'vstinner'
components = ['Interpreter Core', 'Tests']
creation = <Date 2011-02-16.14:25:35.643>
creator = 'sable'
dependencies = []
files = ['20772', '21714', '21716', '21724', '21734', '21744', '21786']
hgrepos = []
issue_num = 11223
keywords = ['patch']
message_count = 41.0
messages = ['128668', '128669', '128670', '128672', '128674', '128675', '128676', '128677', '128678', '128680', '128681', '128718', '128719', '128720', '128721', '128722', '128723', '130755', '134028', '134031', '134037', '134049', '134068', '134069', '134070', '134104', '134105', '134106', '134107', '134108', '134131', '134135', '134189', '134191', '134435', '134484', '134505', '134859', '138862', '138863', '138864']
nosy_count = 9.0
nosy_names = ['gregory.p.smith', 'db3l', 'pitrou', 'vstinner', 'sable', 'benjamin.peterson', 'rnk', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11223'
versions = ['Python 3.2']

[sable]

The test test_lock_acquire_interruption in test_threadsignals.py will lock forever on AIX 5.3.
It works fine on 6.1.

truss shows it is stuck in thread_tsleep:
$ truss -p 1404986
thread_tsleep(0, 0x00000000, 0x00000000, 0x00000000) (sleeping...)

[pitrou]

We have the same issue on our FreeBSD 6.4 buildbot:
http://python.org/dev/buildbot/all/builders/x86%20FreeBSD%203.x/builds/1216/steps/test/logs/stdio
(FreeBSD 7.2 is fine)

Can you tell me what the values of _POSIX_SEMAPHORES and HAVE_SEM_TIMEDWAIT are (or whether they are undefined)?

[sable]

$ grep SEM pyconfig.h
#define HAVE_BROKEN_POSIX_SEMAPHORES 1
/* #undef HAVE_BROKEN_SEM_GETVALUE */
#define HAVE_SEM_GETVALUE 1
#define HAVE_SEM_OPEN 1
#define HAVE_SEM_TIMEDWAIT 1
#define HAVE_SEM_UNLINK 1
/* #undef POSIX_SEMAPHORES_NOT_ENABLED */
#ifndef _POSIX_PTHREAD_SEMANTICS
# define _POSIX_PTHREAD_SEMANTICS 1

[pitrou]

Ok, the problem is when the condition variable-based implementation is chosen in Python/thread_pthread.h.
It is actually reproduceable under Linux by adding #undef USE_SEMAPHORES just after #define USE_SEMAPHORES.
The problem is that, contrary to sem_timedwait(), pthread_cond_wait() doesn't have well-defined behaviour with signals. POSIX simply says:

“If a signal is delivered to a thread waiting for a condition variable, upon return from the signal handler the thread resumes waiting for the condition variable as if it was not interrupted, or it shall return zero due to spurious wakeup.”

So, on those OSes where the semaphore API can't be used, the test can succeed or not depending on how pthread_cond_wait() behaves in the face of signals. It would be better if it didn't hang, though.

(This is not a regression, by the way. Locks were totally uninterruptible under POSIX until 3.2, and the tests weren't there. The new tests were introduced in r87292 (see bpo-8844))

[pitrou]

Here is a patch that makes the test fail rather than hang. Sébastien, can you try it out?

[sable]

Thanks Antoine, the patch solved the problem.

I have other tests locking semi-randomly on AIX 5.3 and/or AIX 6.1, like test_socket and test_io. That may be related.

[pitrou]

I have other tests locking semi-randomly on AIX 5.3 and/or AIX 6.1,
like test_socket and test_io. That may be related.

Which test cases exactly?

[sable]

Well I don't know precisely:

• when running make buildbottest, test_socket will systematically hang on AIX 5.3 and 6.1.

• but when running ./python -Wd -E -bb Lib/test/test_socket.py, the tests complete rather quickly (with one failure cf 11192).

Ran 158 tests in 16.867s

FAILED (failures=1, skipped=4)

I will try with pdb or something.

[pitrou]

Well I don't know precisely:

• when running make buildbottest, test_socket will systematically hang
  on AIX 5.3 and 6.1.

• but when running ./python -Wd -E -bb Lib/test/test_socket.py, the
  tests complete rather quickly (with one failure cf 11192).

You can run the whole test suite in verbose mode. This will pollute your
terminal a lot, but you'll see exactly where it hangs.

Try something like "./python -E -bb -m test.regrtest -uall -v"

[vstinner]

I will try with pdb or something.

You can also try to attach gdb to the running process: with
python-gdb.py, you have nice py-* commands.

Or if you don't have gdb7, you may try my faulthandler module: you will
have to modify the source code (eg. Lib/test/regrtest.py) to add at the
top:

   import faulthandler, signal; faulthandler.register(signal.SIGUSR1)

Then you can display the current Python backtrace by sending a SIGUSR1
signal to the running Python process (eg. killall -USR1 python).

https://github.com/haypo/faulthandler/wiki

[sable]

test_socket didnot lock this time (I will retry next).

But test_subprocess.ProcessTestCasePOSIXPurePython.test_leaking_fds_on_error did.

Looping forever on the following syscall:

pipe(0x2FF1FD88) = 0
kfcntl(15, F_GETFD, 0x00000000) = 0
kfcntl(15, F_SETFD, 0x00000001) = 0
kfcntl(17, F_GETFD, 0x00000000) = 0
kfcntl(17, F_SETFD, 0x00000001) = 0
pipe(0x2FF1FD88) = 0
kfcntl(18, F_GETFD, 0x00000000) = 0
kfcntl(18, F_SETFD, 0x00000001) = 0
kfcntl(19, F_GETFD, 0x00000000) = 0
kfcntl(19, F_SETFD, 0x00000001) = 0
fstatx(15, 0x2FF1FAEC, 76, 0) = 0
kioctl(15, 22528, 0x00000000, 0x00000000) Err#25 ENOTTY
fstatx(15, 0x2FF1FD7C, 76, 0) = 0
lseek(15, 0, 1) Err#29 ESPIPE
fstatx(18, 0x2FF1FAEC, 76, 0) = 0
kioctl(18, 22528, 0x00000000, 0x00000000) Err#25 ENOTTY
fstatx(18, 0x2FF1FD7C, 76, 0) = 0
lseek(18, 0, 1) Err#29 ESPIPE
pipe(0x2FF1FD08) = 0
kfcntl(20, F_GETFD, 0x00000000) = 0
kfcntl(20, F_SETFD, 0x00000001) = 0
kfcntl(21, F_GETFD, 0x00000000) = 0
kfcntl(21, F_SETFD, 0x00000001) = 0
sigprocmask(0, 0xF029C148, 0xF029C150) = 0
kfork() = 2011158
thread_setmymask_fast(0x00000000, 0x00000000, 0x00000000, 0xD0504100, 0x00000000, 0x1216F00B, 0x1216F00B, 0x00000000) = 0x00000000
close(21) = 0
close(17) = 0
close(19) = 0
kread(20, " O S E r r o r : 2 :\0 i".., 50000) = 10
kread(20, 0x33041C38, 50000) = 0
close(20) = 0
kwaitpid(0x2FF1FB50, 2011158, 4, 0x00000000, 0x00000000) = 2011158
close(15) = 0
close(18) = 0
close(15) Err#9 EBADF
close(18) Err#9 EBADF
pipe(0x2FF1FD88) = 0

[sable]

Here is what faulthandler reports when I trigger it as Python is locked in test_socket:

"""
[108/349] test_socket
Traceback (most recent call first):
  File "/san_cis/home/cis/.buildbot/python-aix6/3.x.phenix.xlc/build/Lib/test/fork_wait.py", line 30 in f
""" (sic)

It is coherent with what truss tells:
_getpid() = 1474634
_select(0, 0x00000000, 0x00000000, 0x00000000, 0x317535F0) = 0
_getpid() = 1474634
_select(0, 0x00000000, 0x00000000, 0x00000000, 0x315E35F0) = 0
....

And we can find this comment in the file:
# waitpid() shouldn't hang, but some of the buildbots seem to hang
# in the forking tests. This is an attempt to fix the problem.

[sable]

faulthandler is great by the way! I will use that in my application.

[vstinner]

Here is what faulthandler reports when I trigger it as Python
is locked in test_socket:
...
File ".../Lib/test/fork_wait.py", line 30 in f

faulthandler doesn't print the source code line (yet?). Here is the code:

class ForkWait(unittest.TestCase):
    ...
    def f(self, id):
        while not self.stop:
            self.alive[id] = os.getpid()
            try:
                time.sleep(SHORTSLEEP) <~~~ here
            except IOError:
                pass

ForkWait.f() is used by ForkWait.test_wait() which creates 4 threads. You may use all_threads=True option of faulthandler to get the backtrace of all threads.

Because ForkWait.test_wait() uses fork, you may also need to dump the backtrace of two processes.

Debug threads+multiple processes is something horrible :-)

[sable]

At the time of the lock, there is only one python process running. So I guess the other processes have already left but this is not correctly detected.

I will try again with all_threads=True.

[sable]

I kinda feel this is related to bpo-11185 which I reported earlier.

[pitrou]

Could you open another issue? (or post in bpo-11185 above)

[python-dev]

New changeset 5d0d488cbca8 by Antoine Pitrou in branch '3.2':
Issue bpo-11223: Fix test_threadsignals to fail, not hang, when the
http://hg.python.org/cpython/rev/5d0d488cbca8

[vstinner]

“If a signal is delivered to a thread waiting for a condition variable,
upon return from the signal handler the thread resumes waiting for the condition
variable as if it was not interrupted, or it shall return zero due to spurious
wakeup.”

Confirmed on Linux (kernel 2.6.37, eglibc 2.11.2) using strace:
--------
10:42:59.942455 futex(0x173fabc, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 1, {1303202584, 942432000}, ffffffff) = ? ERESTART_RESTARTBLOCK (To be restarted)
10:43:00.941777 --- SIGALRM (Alarm clock) @ 0 (0) ---
10:43:00.941848 rt_sigreturn(0x1462eac) = -1 EINTR (Interrupted system call)
10:43:00.941926 futex(0x173fabc, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 1, {1303202584, 942432000}, ffffffff) = -1 ETIMEDOUT (Connection timed out)
10:43:04.942650 futex(0x173fae8, FUTEX_WAKE_PRIVATE, 1) = 0
--------
pthread_cond_timedwait() is interrupted: the system call returns ERESTART_RESTARTBLOCK (To be restarted), and then the system call is restarted.

I think that we should just skip the test if Python uses pthread without semaphore and pthread_cond_timedwait() restarts if it is interrupted by a signal. Attached C program checks if pthread_cond_timedwait() does restart or not. On Linux, it should be compiled using:
gcc -pthread -lrt pthread_cond_timedwait_signal.c -o pthread_cond_timedwait_signal

I don't know how to integrate pthread_cond_timedwait_signal.c into configure.in. I don't know if any OS does NOT restart pthread_cond_timedwait() after an interruption, so it's maybe simpler to always skip test_lock_acquire_interruption() and test_rlock_acquire_interruption() of test_threadsignals if Python uses pthread without semaphore.

[vstinner]

The problem is that I don't know how to check in test_threadsignals which thread implementation is used in Python. It would be nice to have some functions providing such information in the sys or sysconfig module, maybe something like sys.float_info.

For example, sys._thread_info can be a dict like {'name': 'pthread', 'use_semaphore': True}.

Keys:

• name: cthread, lwp, nt, os2, pth, pthread, sgi, solaris or wince (sgi, lwp, pth, cthread may be removed from Python 3.3: bpo-11876)
• maxproc: max # of threads that can be started (SGI only)
• stacksize: default stacksize for a thread (lwp, pthread and os2 only)
• min_stacksize: minimum stacksize for a thread (pthread, nt and os2 only)
• max_stacksize: maximum stacksize for a thread (nt and os2 only)

thread_pthread.h contains many #ifdef which may be interesting to have in thread_info:

• if _HAVE_BSDI is defined, don't use pthread_init()
• if THREAD_STACK_SIZE is defined, set the size of the thread stack
• if PTHREAD_SYSTEM_SCHED_SUPPORTED is defined: set scope to PTHREAD_SCOPE_SYSTEM
• if (defined(_POSIX_SEMAPHORES) && defined(HAVE_BROKEN_POSIX_SEMAPHORES) && defined(HAVE_SEM_TIMEDWAIT)): use semaphore

Well, the most important informations for this issue is the name of the thread implementation (pthread) and know if pthread semaphore are used or not.

[vstinner]

threading_info.patch:

• Add thread._info() function -> dict with 'name' and 'use_semaphores' (pthread only) keys
• Skip test_lock_acquire_interruption() and test_rlock_acquire_interruption() if Python uses pthread without semaphores

thread._info() creates a new dict at each call.

[pitrou]

threading_info.patch:

• Add thread._info() function -> dict with 'name' and
  'use_semaphores' (pthread only) keys

Most of these names will be removed in 3.3:
http://bugs.python.org/issue11863

[vstinner]

Most of these names will be removed in 3.3:
http://bugs.python.org/issue11863

Yeah, I know. My patch should be updated if bpo-11863 is fixed before this issue.

Updated patch:

• add 'pthread_version' key to threading._info()
• test_os uses threading._info() to check if we are using linuxthreads

[vstinner]

Example on my Debian Sid:

>>> threading._info()
{'pthread_version': 'NPTL 2.11.2', 'use_semaphores': True, 'name': 'pthread'}

[pitrou]

Yeah, I know. My patch should be updated if bpo-11863 is fixed before this issue.

Updated patch:

• add 'pthread_version' key to threading._info()
• test_os uses threading._info() to check if we are using linuxthreads

The path which sets "pthread_version" should be inside "#ifdef
_POSIX_THREADS".

Also, some comments about the doc:

• you need a "versionadded" tag
• "use_semaphores" should explain that these semaphores are used for
  locks; also the alternative is "use mutexes and condition variables",
  not "use mutexes"
• you should not document unsupported platform names ("lwp", etc.); they
  are already disabled (#error)

[vstinner]

The path which sets "pthread_version" should be inside "#ifdef
_POSIX_THREADS".
Also, some comments about the doc:

• you need a "versionadded" tag

Right, fixed.

• "use_semaphores" should explain that these semaphores are used for
  locks; also the alternative is "use mutexes and condition variables",
  not "use mutexes"

Hum, a boolean was not a good idea. I replaced this key by:

• 'lock' (string): name of the lock implementation

  • 'semaphore': a lock uses a semaphore
  • 'mutex+cond': a lock uses a mutex and a condition variable

I also renamed 'name' key to 'thread', so 'thread' is the name of the
*thread* implementation, and 'lock' is the name of the *lock*
implementation.

For example, test_threadsignals now uses:
-----------

USING_PTHREAD_COND = (info['thread'] == 'pthread'
                      and info['lock'] == 'mutex+cond')
...
@unittest.skipIf(USING_PTHREAD_COND,
                 'POSIX condition variables cannot be interrupted')

---

I think that the test is more clear than:
-----------
@unittest.skipIf(info['name'] == 'pthread' and not
info['use_semaphores'],
'POSIX mutexes cannot be interrupted')
-----------

(the message was wrong, the interrupt issue is on the condition, not on
the mutex)

• you should not document unsupported platform names ("lwp", etc.); they
  are already disabled (#error)

Oh, I didn't realize that they were already unsupported as a compilation
error. Ok, fixed. I also removed "wince": I realized that thread_wince.h
is not used (see bpo-11863).

The new patch (version 3) marks also PyThread_Info() as private (rename
it to _PyThread_Info).

[pitrou]

I also renamed 'name' key to 'thread', so 'thread' is the name of the
*thread* implementation, and 'lock' is the name of the *lock*
implementation.

Not that I want to bikeshed, but I think 'name' was ok (since you get
the dict by calling threading._info(), it's obvious it has to do with
threading).
'lock_implementation' would be better than 'lock', OTOH.

Also, the 'pthread_version' documentation should state that it is
optional (only works on GNU).

[python-dev]

New changeset 383a7301616b by Victor Stinner in branch 'default':
Issue bpo-11223: Add threading._info() function providing informations about the
http://hg.python.org/cpython/rev/383a7301616b

[vstinner]

Let's wait 6 hours for "x86 FreeBSD 6.4 3.x":
http://www.python.org/dev/buildbot/all/builders/x86%20FreeBSD%206.4%203.x

The first build including my fix (383a7301616b) should be:
http://www.python.org/dev/buildbot/all/builders/x86%20FreeBSD%206.4%203.x/builds/1395

[python-dev]

New changeset bc1c6bd7eeb0 by Victor Stinner in branch 'default':
Issue bpo-11223: fix test_dummy_threading, add _dummy_thread.info()
http://hg.python.org/cpython/rev/bc1c6bd7eeb0

[vstinner]

"Builder x86 FreeBSD 6.4 3.x Build bpo-1394. Results: Build successful"
http://www.python.org/dev/buildbot/all/builders/x86%20FreeBSD%206.4%203.x/builds/1394

Great! It should be the first success on this buildbot since... 4 months (r87292, issue bpo-8844))? Let's close this issue.

[python-dev]

New changeset 64008d17fb54 by Victor Stinner in branch 'default':
Issue bpo-11223: fix compiler warnings
http://hg.python.org/cpython/rev/64008d17fb54

[vstinner]

sys_thread_info.patch:

• Replace threading._info() by sys.thread_info: it now always have 3 values, but all values are optional (can be None). sys.thread_info.name is None if Python is compiled without threads.
• Reorder sys internal doc (Static objects) and replace "dict" by "struct sequence" for float_info
• Rename _PyThread_Info() to PyThread_GetInfo() (consistent name with PyFloat_GetInfo() and PyLong_GetInfo())
• Always compile thread.c, but add #ifdef WITH_THREAD around most the file (except PyThread_GetInfo())

Example:

>>> sys.thread_info
sys.thread_info(name='pthread', lock='semaphore', version='NPTL 2.11.2')

[pitrou]

• Rename _PyThread_Info() to PyThread_GetInfo() (consistent name with
  PyFloat_GetInfo() and PyLong_GetInfo())

I don't think we want that API to be public, so it should be
_PyThread_GetInfo().

• Always compile thread.c, but add #ifdef WITH_THREAD around most the
  file (except PyThread_GetInfo())

What's the point? Sounds like pointless complication.

[vstinner]

> - Rename _PyThread_Info() to PyThread_GetInfo() (consistent name with
> PyFloat_GetInfo() and PyLong_GetInfo())

I don't think we want that API to be public, so it should be
_PyThread_GetInfo().

Why should it be private in C, but not in Python?

Why should it be private whereas PyLong_GetInfo() and PyFloat_GetInfo()
are public?

> - Always compile thread.c, but add #ifdef WITH_THREAD around most the
> file (except PyThread_GetInfo())

What's the point? Sounds like pointless complication.

Complication? It does *simplify* configure.in.

I don't want to create a new file just for a small function.

[pitrou]

Le mardi 26 avril 2011 à 01:34 +0000, STINNER Victor a écrit :

STINNER Victor <victor.stinner@haypocalc.com> added the comment:

> > - Rename _PyThread_Info() to PyThread_GetInfo() (consistent name with
> > PyFloat_GetInfo() and PyLong_GetInfo())
>
> I don't think we want that API to be public, so it should be
> _PyThread_GetInfo().

Why should it be private in C, but not in Python?

Well, if it's called _info() in Python, it's private too!

> > - Always compile thread.c, but add #ifdef WITH_THREAD around most the
> > file (except PyThread_GetInfo())
>
> What's the point? Sounds like pointless complication.

Complication? It does *simplify* configure.in.

I don't want to create a new file just for a small function.

What I mean is that the "#ifdef WITH_THREAD" could be done in
sysmodule.c rather than in thread.c

Also, when Python is compiled without threads, I don't think thread_info
should be a structseq. It should probably be None instead.

Another small thing: your doc says "name" is optional, but it shouldn't.

[vstinner]

Well, if it's called _info() in Python, it's private too!

My patch replaces thread._info() by sys.thread_info: it becomes public.

What I mean is that the "#ifdef WITH_THREAD" could be done in
sysmodule.c rather than in thread.c

PyThread_GetInfo() requires some informations that are only available at the end of thread.c: USE_SEMAPHORES define from thread_pthread.h and PYTHREAD_NAME from thread.c. It is easier to define PyThread_GetInfo() here, instead of giving access to these defines outside thread.c (and these defines should remaing private).

Another small thing: your doc says "name" is optional, but it shouldn't.

By optional I mean that its value is None if Python is compiled without threads.

Also, when Python is compiled without threads,
I don't think thread_info should be a structseq.
It should probably be None instead.

Terry Reedy proposed an empty string for name if Python is compiled without threads. Antoine suggested None instead of an empty string for lock and version fields, so I chose to use also None for None.

But yes, I like the idea of sys.thread_info being None.

--

Updated patch (sys_thread_info-2.patch):

• sys.thread_info is None if Python is compiled without threads
• sys.thread_info.name is no more optional
• change the documentation of the lock and version fields
• fix test_os (version is now a attribute and no more a key of a dict)

[python-dev]

New changeset 2b21fcf3d9a9 by Victor Stinner in branch 'default':
Issue bpo-11223: Replace threading._info() by sys.thread_info
http://hg.python.org/cpython/rev/2b21fcf3d9a9

[python-dev]

New changeset 07655b3dee4f by Victor Stinner in branch '3.2':
Issue bpo-11223: skip test_lock_acquire_interruption() on FreeBSD6
http://hg.python.org/cpython/rev/07655b3dee4f

[python-dev]

New changeset 3f18a03a2a1e by Victor Stinner in branch 'default':
(null merge 3.2 for issue bpo-11223) python 3.3 has already a better fix
http://hg.python.org/cpython/rev/3f18a03a2a1e

[python-dev]

New changeset e5183f16c49d by Victor Stinner in branch '3.2':
Issue bpo-11223: skip also test_rlock_acquire_interruption() on FreeBSD6
http://hg.python.org/cpython/rev/e5183f16c49d

New changeset 54fb77e0762c by Victor Stinner in branch 'default':
(null merge 3.2 for issue bpo-11223) python 3.3 has already a better fix
http://hg.python.org/cpython/rev/54fb77e0762c