Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust test_concurrent_futures to run more of its tests if multiprocessing.synchronize is missing #84869

Closed
paulproteus mannequin opened this issue May 19, 2020 · 5 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@paulproteus
Copy link
Mannequin

paulproteus mannequin commented May 19, 2020

BPO 40692
Nosy @brianquinlan, @pitrou, @vstinner, @asvetlov, @zware, @1st1, @applio, @pablogsal, @paulproteus
PRs
  • bpo-40692: Run more test_concurrent_futures tests #20239
  • 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 2021-02-08.03:16:20.239>
    created_at = <Date 2020-05-19.21:07:44.662>
    labels = ['tests', 'type-feature', 'library', '3.10']
    title = 'Adjust test_concurrent_futures to run more of its tests if multiprocessing.synchronize is missing'
    updated_at = <Date 2021-02-08.03:16:20.238>
    user = 'https://github.com/paulproteus'

    bugs.python.org fields:

    activity = <Date 2021-02-08.03:16:20.238>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-02-08.03:16:20.239>
    closer = 'pablogsal'
    components = ['Library (Lib)', 'Tests']
    creation = <Date 2020-05-19.21:07:44.662>
    creator = 'asheesh'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40692
    keywords = ['patch']
    message_count = 5.0
    messages = ['369393', '369633', '370674', '385717', '386613']
    nosy_count = 10.0
    nosy_names = ['bquinlan', 'pitrou', 'vstinner', 'asvetlov', 'python-dev', 'zach.ware', 'yselivanov', 'davin', 'pablogsal', 'asheesh']
    pr_nums = ['20239']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue40692'
    versions = ['Python 3.10']

    @paulproteus
    Copy link
    Mannequin Author

    paulproteus mannequin commented May 19, 2020

    Some parts of test_concurrent_futures make sense to run even on Python builds where multiprocessing.synchronize is absent. At the moment, test_concurrent_futures.py skips all tests in the file if multiprocessing.synchronize is missing from the Python build. I have a patch to enable more tests, which I'll submit as a GitHub pull request.

    The reason I want this patch is while working on CPython on Android, I saw that test_concurrent_futures refused to run its test suite, and I became very afraid that this meant that asyncio on CPython on Android was doomed. In fact, I was afraid for no reason. I want future people porting CPython to be less scared than I was. :)

    I discovered that the only part of concurrent.futures that requires multiprocessing.synchronize is concurrent.futures.ProcessPoolExecutor.

    concurrent.futures.ProcessPoolExecutor already has a way to check at runtime if it can work on the host system, so this patch leans on that.

    This patch fixes a few other tests to skip themselves in the case that ProcessPoolExecutor cannot function.

    I tested this by building CPython (with my patch) with adding a configure flag on macOS:

    $ ./configure --with-pydebug --with-openssl=$(brew --prefix openssl) ac_cv_posix_semaphores_enabled=no

    Why multiprocessing.synchronize is missing
    ------------------------------------------

    multiprocessing.synchronize imports _multiprocessing.SemLock, which is based on a libc function called sem_open(), which implements named semaphores, which allows the creation of lock-esque counters that can be used from multiple processes. multiprocessing.c currently only creates the SemLock property if sem_open() is considered "working" through a check in ./configure; I've short-circuited that check above to be a "no".

    On Android, sem_open() exists but simply returns ENOSYS; see its implementation here: https://android.googlesource.com/platform/external/pthreads/+/master/sem_open.c

    Android isn't the only platform missing sem_open()

    Past work
    ---------

    bpo-3770 is an older conversation about a related issue. bpo-3770 has also attracted comments from someone who wants CPython's test suite to run to completion on their platform lacking sem_open() and therefore lacking multiprocessing.synchronize. This patch should solve their problem, I hope.

    Future work
    -----------

    Automated testing: In order to prevent regressions, I'm interested in hosting a buildbot for at least one year, hopefully in perpetuity, on which we call ./configure with ac_cv_posix_semaphores_enabled=no.

    I'd like to improve the error message when the user imports multiprocessing.synchronize so it points at some new documentation text that I (or someone) adds to CPython, rather than the issue number. I'm OK taking on the work of writing those docs in a follow-up issue if we merge this.

    Thanks
    ------

    Thanks to Zachary Ware for discussing this with me in an CPython contributor office hours time slot before I filed the issue. Thanks to Dr. Russell Keith-Magee, Glyph Lefkowitz, Nathaniel Smith, and Geoffrey Thomas for informal conversations and technical research.

    @paulproteus paulproteus mannequin added tests Tests in the Lib/test dir 3.9 only security fixes topic-asyncio type-feature A feature request or enhancement labels May 19, 2020
    @zware
    Copy link
    Member

    zware commented May 22, 2020

    Adding multiprocessing and concurrent.futures experts.

    @zware zware added 3.10 only security fixes stdlib Python modules in the Lib dir tests Tests in the Lib/test dir and removed tests Tests in the Lib/test dir 3.9 only security fixes topic-asyncio labels May 22, 2020
    @vstinner
    Copy link
    Member

    vstinner commented Jun 3, 2020

    Automated testing: In order to prevent regressions, I'm interested in hosting a buildbot for at least one year, hopefully in perpetuity, on which we call ./configure with ac_cv_posix_semaphores_enabled=no.

    Can't you tune an unit test to prevent multiprocessing.synchronize to be imported? For example, sys.modules['multiprocessing.synchronize'] = None ensures that "import multiprocessing.synchronize" fails, even if the module exists and works.

    $ python3
    Python 3.8.3 (default, May 15 2020, 00:00:00) 
    >>> import sys
    >>> sys.modules['multiprocessing.synchronize'] = None
    >>> import multiprocessing.synchronize
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ModuleNotFoundError: import of multiprocessing.synchronize halted; None in sys.modules

    A whole buildbot sounds like an overkill solution to this problem.

    On the other hand, multiprocessing tests are already ones of the slowest tests of the test suite *because* they try to test all possible combinations.

    @vstinner
    Copy link
    Member

    Is there any update on this issue? If not, I suggest to close it. It doesn't seem to consider enough people and nobody seems to want to implement it.

    See also: python/buildmaster-config#203

    @pablogsal
    Copy link
    Member

    New changeset bf2e7e5 by Asheesh Laroia in branch 'master':
    bpo-40692: Run more test_concurrent_futures tests (GH-20239)
    bf2e7e5

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants