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

bpo-31783: Fix a race condition creating workers during shutdown #13171

Merged
merged 4 commits into from
Jun 28, 2019

Conversation

brianquinlan
Copy link
Contributor

@brianquinlan brianquinlan commented May 7, 2019

@markshannon
Copy link
Member

Can you add a test for this, thanks.

@brianquinlan
Copy link
Contributor Author

brianquinlan commented May 8, 2019

The race condition occurs with the following set of context switches:

Main Thread                           Thread  #1
========= .                         ========
1.                                   ThreadPoolExecutor.submit()
                                        ...
                                        if _shutdown:

                    <=====

2. thread._python_exit
      _shutdown = true
      items = list(_threads_queues.items())

                    =====>

3.                                      ...
                                         _threads_queues[t] = self._work_queue

                    <=====

4. 
    for t, q in items:
        q.put(None)
    for t, q in items:
        t.join()

At this point there is a new thread added to _threads_queues but thread._python_exit won't join it because it was added to _threads_queues after a copy was made (i.e.
items = list(_threads_queues.items()))

The bug has a patch with time.sleep() to force the pathological sequencing and a test script to demonstrate the problem.

@brianquinlan
Copy link
Contributor Author

I don't think writing a test for this would be feasible because provoking the race condition requires very specific timing that, at least on my machine, I can't provoke without adding sleep calls to the code.

@brianquinlan brianquinlan reopened this Jun 28, 2019
@brianquinlan brianquinlan merged commit 242c26f into python:master Jun 28, 2019
@bedevere-bot
Copy link

@brianquinlan: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @brianquinlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @brianquinlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Sorry, @brianquinlan, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 242c26f53edb965e9808dd918089e664c0223407 3.8

@miss-islington
Copy link
Contributor

Sorry @brianquinlan, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 242c26f53edb965e9808dd918089e664c0223407 3.7

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
…hon#13171)

* bpo-31783: Fix a race condition while creating workers during interpreter shutdown

* 📜🤖 Added by blurb_it.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…hon#13171)

* bpo-31783: Fix a race condition while creating workers during interpreter shutdown

* 📜🤖 Added by blurb_it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants