Skip to content

Promise the long-time truth that args=list works #89898

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

Closed
tim-one opened this issue Nov 6, 2021 · 8 comments
Closed

Promise the long-time truth that args=list works #89898

tim-one opened this issue Nov 6, 2021 · 8 comments
Labels
3.11 only security fixes docs Documentation in the Doc dir easy type-bug An unexpected behavior, bug, or error

Comments

@tim-one
Copy link
Member

tim-one commented Nov 6, 2021

BPO 45735
Nosy @tim-one, @rhettinger, @serhiy-storchaka, @CharlieZhao95
PRs
  • bpo-45735: Promised that lists can be used for Thread args. #29437
  • bpo-45735: Promise the long-time truth that args=list works #30982
  • 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 = None
    created_at = <Date 2021-11-06.04:02:01.899>
    labels = ['easy', '3.11', 'type-bug', 'docs']
    title = 'Promise the long-time truth that `args=list` works'
    updated_at = <Date 2022-02-26.04:17:24.028>
    user = 'https://github.com/tim-one'

    bugs.python.org fields:

    activity = <Date 2022-02-26.04:17:24.028>
    actor = 'tim.peters'
    assignee = 'docs@python'
    closed = False
    closed_date = None
    closer = None
    components = ['Documentation']
    creation = <Date 2021-11-06.04:02:01.899>
    creator = 'tim.peters'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45735
    keywords = ['patch', 'easy']
    message_count = 7.0
    messages = ['405846', '405851', '405867', '406145', '411823', '411825', '414067']
    nosy_count = 5.0
    nosy_names = ['tim.peters', 'rhettinger', 'docs@python', 'serhiy.storchaka', 'CharlieZhao']
    pr_nums = ['29437', '30982']
    priority = 'low'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45735'
    versions = ['Python 3.11']

    @tim-one
    Copy link
    Member Author

    tim-one commented Nov 6, 2021

    A number of contexts allow specifying a tuple of arguments to be passed later to a function. The Thread constructor is a fine example, and happened to come up (again! for me) here today:

    https://stackoverflow.com/questions/69858950/why-do-we-have-to-add-comma-in-args-in-python-multithreading/69859068

    This often confuses especially newbies, because the function they intend to parallelize often takes only a single argument, and Python's syntax for a 1-element tuple actually _requires_ parentheses in the context of an argument list, with a naked trailing comma:

    t = threading.Thread(target=access, args=(thread_number,))

    It "looks weird" to people.

    I'm not suggesting to change that, but instead to officially bless the workaround I've seen very often in real code: use a list instead.

    t = threading.Thread(target=access, args=[thread_number])

    Nobody scratches their head over what that means.

    CPython's implementations typically couldn't care less what kind of sequence is used, and none that I'm aware of verify that it's specifically a tuple. The implementations just go on to do some simple variation of

        self.target(*self.args)

    Tuple or list makes no real difference. I'm not really keen to immortalize the "any sequence type whatsoever that just happens to work" implementation behavior, but am keen to promise that a list specifically will work. A lot of code already relies on it.

    @tim-one tim-one added the 3.11 only security fixes label Nov 6, 2021
    @tim-one tim-one added docs Documentation in the Doc dir easy type-bug An unexpected behavior, bug, or error 3.11 only security fixes labels Nov 6, 2021
    @tim-one tim-one added docs Documentation in the Doc dir easy type-bug An unexpected behavior, bug, or error labels Nov 6, 2021
    @tim-one tim-one changed the title Promise that the long-time truth that args=list works Promise the long-time truth that args=list works Nov 6, 2021
    @tim-one tim-one changed the title Promise that the long-time truth that args=list works Promise the long-time truth that args=list works Nov 6, 2021
    @serhiy-storchaka
    Copy link
    Member

    There is a difference if you modify the arguments list after creating a thread.

        args = [1]
        t = threading.Thread(target=access, args=args)
        args[0] = 2
        t.start()

    Would it call access(1) or access(2)?

    @tim-one
    Copy link
    Member Author

    tim-one commented Nov 6, 2021

    Serhiy, we haven't documented such stuff, and, indeed, I've been burned by it but much more often in the case of multiprocessing.Process. But note that I'm SWAPPING the order of your last two lines. In the original, you mutated the argument _before_ starting any parallel work, so "of course" the new worker will see the mutation:

        def access(xs):
            print(xs)
    
        args = ([1],)
        t = multiprocessing.Process(target=access, args=args)
        t.start() # start parallel work before mutating
        args[0][0] = 2

    Does that print [1] or [2]? Passing a tuple in no way prevents mutations to mutable objects the tuple contains.

    When the docs are silent, "implementation defined" rules. Whether you use threading or multiprocessing in the altered example above, the result printed simply isn't defined - it's a race between the main thread doing the mutation and the "parallel part" accessing the mutated object.

    This is subtler in the multiprocessing context, though, because the relevant "parallel part" is really the hidden thread that pickles the argument list to send to the worker. That effectively makes a deep copy. But it's still a race, just not one visible from staring at the Python code. In the threading case, no copies are made.

    @tim-one
    Copy link
    Member Author

    tim-one commented Nov 11, 2021

    Changed stage back to "needs patch", since Raymond appears to have closed his PR. Raymond, what's up with that?

    @CharlieZhao95
    Copy link
    Mannequin

    CharlieZhao95 mannequin commented Jan 27, 2022

    I'd like to work on this issue recently if it's still needed.

    According to suggestions in PR:#29437, I will:

    • Add doc example for Thread function;
    • Add some test cases for checking the validity of list args;
    • Repeat the above works on multiprocessing module.

    @tim-one
    Copy link
    Member Author

    tim-one commented Jan 27, 2022

    Charliz, please do! I have no idea why Raymond just stopped. He even deleted his initial message here, saying "I relied on this for many years. So, yet it would be nice to guarantee it :-)".

    Best I can tell, nothing has changed: lots of people have relied on this behavior for years, and the docs should say it's fine.

    @tim-one
    Copy link
    Member Author

    tim-one commented Feb 26, 2022

    New changeset e466faa by Charlie Zhao in branch 'main':
    bpo-45735: Promise the long-time truth that args=list works (GH-30982)
    e466faa

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @tim-one
    Copy link
    Member Author

    tim-one commented Apr 11, 2022

    @CharlieZhao95's doc change was merged, so closing this issue.

    @tim-one tim-one closed this as completed Apr 11, 2022
    vstinner added a commit that referenced this issue May 17, 2022
    Join the thread to not leak threads running in the background to the
    next test.
    
    Fix the following warning on the "AMD64 FreeBSD Shared 3.11"
    buildbot:
    
    test_args_argument (test.test_threading.ThreadTests.test_args_argument) ...
    Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
    Warning -- Dangling thread: <_MainThread(MainThread, started 35026161664)>
    Warning -- Dangling thread: <Thread(Thread-134 (<lambda>), started 35314998016)>
    ok
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 17, 2022
    Join the thread to not leak threads running in the background to the
    next test.
    
    Fix the following warning on the "AMD64 FreeBSD Shared 3.11"
    buildbot:
    
    test_args_argument (test.test_threading.ThreadTests.test_args_argument) ...
    Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
    Warning -- Dangling thread: <_MainThread(MainThread, started 35026161664)>
    Warning -- Dangling thread: <Thread(Thread-134 (<lambda>), started 35314998016)>
    ok
    (cherry picked from commit 970efae)
    
    Co-authored-by: Victor Stinner <[email protected]>
    miss-islington added a commit that referenced this issue May 17, 2022
    Join the thread to not leak threads running in the background to the
    next test.
    
    Fix the following warning on the "AMD64 FreeBSD Shared 3.11"
    buildbot:
    
    test_args_argument (test.test_threading.ThreadTests.test_args_argument) ...
    Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
    Warning -- Dangling thread: <_MainThread(MainThread, started 35026161664)>
    Warning -- Dangling thread: <Thread(Thread-134 (<lambda>), started 35314998016)>
    ok
    (cherry picked from commit 970efae)
    
    Co-authored-by: Victor Stinner <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes docs Documentation in the Doc dir easy type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants