Re-enable threading test on macOS

[ronaldoussoren]

BPO	18049
Nosy	@ronaldoussoren, @vstinner, @ned-deily, @zooba, @matrixise, @aixtools, @miss-islington, @tirkarthi, @aeros
PRs	bpo-18049: Sync thread stack size to main thread size on macOS #14748 [3.8] bpo-18049: Sync thread stack size to main thread size on macOS (GH-14748) #15065 bpo-18049: Reintroduce platform guard in recursion limit test #15075 bpo-18049: Define THREAD_STACK_SIZE for AIX to pass default recursion limit test #15081 [3.8] bpo-18049: Define THREAD_STACK_SIZE for AIX to pass default recursion limit test (GH-15081) #15089 [3.8] bpo-18049: Define THREAD_STACK_SIZE for AIX to pass default recursion limit test (GH-15081) #15572
Files	reenable-threading-test.txt

^{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 = 'https://github.com/ronaldoussoren'
closed_at = <Date 2019-08-03.07:20:18.384>
created_at = <Date 2013-05-24.14:27:46.013>
labels = ['OS-mac', 'type-bug', '3.8', '3.9']
title = 'Re-enable threading test on macOS'
updated_at = <Date 2019-09-06.14:30:57.071>
user = 'https://github.com/ronaldoussoren'

bugs.python.org fields:

activity = <Date 2019-09-06.14:30:57.071>
actor = 'ronaldoussoren'
assignee = 'ronaldoussoren'
closed = True
closed_date = <Date 2019-08-03.07:20:18.384>
closer = 'ronaldoussoren'
components = ['macOS']
creation = <Date 2013-05-24.14:27:46.013>
creator = 'ronaldoussoren'
dependencies = []
files = ['30359']
hgrepos = []
issue_num = 18049
keywords = ['patch', 'needs review']
message_count = 30.0
messages = ['189912', '192389', '270766', '271071', '272116', '323757', '347830', '347868', '347888', '347901', '348839', '348847', '348860', '348875', '348876', '348887', '348891', '348894', '348895', '348896', '348897', '348899', '348900', '348945', '350083', '350718', '351180', '351185', '351195', '351256']
nosy_count = 9.0
nosy_names = ['ronaldoussoren', 'vstinner', 'ned.deily', 'steve.dower', 'matrixise', 'Michael.Felt', 'miss-islington', 'xtreak', 'aeros']
pr_nums = ['14748', '15065', '15075', '15081', '15089', '15572']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18049'
versions = ['Python 3.8', 'Python 3.9']

[ronaldoussoren]

I recently notied that test.test_threading.ThreadingExceptionTests.test_recursion_limit is disabled for debug builds.

The attached patch re-enables this test case and fixes the crash be increasing the stack size for new threads.

The disadvantage of the patch is that is increases the default stack size for all new threads on OSX from 5 to 8 megabytes (for threads created by Python) and that could affect program behavior as fewer threads can be created before running out of memory.

The patch could be updated to check for Py_DEBUG and only increase the stack size for --with-pydebug builds (which are the ones using -O0), as those builds need the increased stack size and debug builds already consume more memory that normal builds. That's not 100% reliable though, as users could also build the with CFLAGS set to -O0 without using --with-pydebug.

I haven't found a way to detect the optimization level with clang or gcc using the preprocessor.

[ronaldoussoren]

I intend to apply this patch to default (and only that branch) after running the testsuite and with a small change: the stack size will be 0x1000000 (double it current value) to sync it with the stack size for the main thread, which was recently set to that value in the default branch.

[matrixise]

for my part, I think you should use a #ifdef PY_DEBUG in your code and increase the default size of the stack for the threads, and only if you are on APPLE and PY_DEBUG mode.

[ronaldoussoren]

The patch increases the stack size for threads to slightly less than size as is used for the main thread, see this fragment in configure.ac:

# Issue bpo-18075: the default maximum stack size (8MBytes) is too
# small for the default recursion limit. Increase the stack size
# to ensure that tests don't crash
LINKFORSHARED="-Wl,-stack_size,1000000 $LINKFORSHARED"

Maybe the patch should be updated to use the same size?

The disadvantage of any increase of size is that this reduces the number of threads that can be used, in particular for 32-bit builds (64-bit builds have more than enough address space)

[matrixise]

yes, maybe, could you provide a patch ?

[ronaldoussoren]

I have a patch for making the stack sizes the same in bpo-34264.

[ronaldoussoren]

I've converted the patch to a pull request (and closed to other issue because that has a patch that is less complete).

I'm not sure if it is acceptable to backport this patch to 3.8 at this time.

[zooba]

I believe this is acceptable to include in 3.8, provided it makes sense for 3.9, so I added the backport tag on the PR.

That said, I'm not making any comment on whether it's appropriate at all :)

For Windows we found some ways to reduce the stack usage of debug builds (IIRC, refactoring some large local variables into their own function) and improved recursion depth that way. Without any data on whether worker threads generally go deeper than the main thread, I would hesitate to say that they ought to be the same, as there is a definite advantage to having a smaller stack by default when you have many worker threads.

But if someone else confidently approves the change, it can go into 3.8.

[ronaldoussoren]

I've done some more testing, and the thread stack size could be smaller than 16MB, but definitely not as small as the value that's currently in thread_pthread.h.

I prefer to have a slightly too large value here because IIRC the relation between the recursion limit and C stack usage is a heuristic.

BTW. It should be possible to implement PyOS_CheckStack() on macOS. There are (undocumented) APIs in the public header files to get the stack base and size for the current thread. These APIs also work for the main thread:

# ---

#include <pthread.h>
#include <stdio.h>

int main(void)
{
        printf("%#lx\n", (long)pthread_get_stacksize_np(pthread_self()));
        printf("%p\n", pthread_get_stackaddr_np(pthread_self()));
}

# ---