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.

Sign up for GitHub

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

Jump to bottom

bpo-36402: Fix threading._shutdown() race condition #13948

Merged
merged 1 commit into from Jun 12, 2019
Merged

bpo-36402: Fix threading._shutdown() race condition #13948

merged 1 commit into from Jun 12, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 10, 2019 •
edited by bedevere-bot

Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of threads get deleted, rather
than just wait until the Python thread completes.

  • Add threading._shutdown_locks set: set of Thread._tstate_lock locks
    of non-daemon threads used by _shutdown() wait until all Python
    thread states get deleted (see Thread._set_tstate_lock()).
  • Add also threading._shutdown_locks_lock to protect access to
    threading._shutdown_locks.

https://bugs.python.org/issue36402

@vstinner
Copy link
Member Author

This PR changes critical code, so I would appreciate multiple reviews :-)

test_threading: test_threads_join_2() was added by commit 7b47699 in 2013, but the test failed randomly since it was added. It's just that failures were ignored until I created https://bugs.python.org/issue36402 last March.

In fact, when the test failed randomly on buildbot (with tests run in parallel), it was fine since test_threading was re-run alone and then the test passed. The buildbot build was seen overall as a success.

The test shows the bug using subinterpreters (Py_EndInterpreter), but I'm quite sure that Py_Finalize() has the same race condition since it also calls threading._shutdown().

This was referenced Jun 10, 2019
Closed
Closed
pitrou
pitrou approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find, and the fix looks correct. +1.

@pitrou pitrou requested a review from tim-one June 11, 2019 13:33
gpshead
gpshead requested changes Jun 11, 2019
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't entirely digested this change yet. It looks good but I don't know if it is enough, I'll need to dig further to convince myself of that. Meanwhile, one issue noted.

Lib/threading.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member Author

I haven't entirely digested this change yet.

Sadly, these things are badly documented. The main issue is that there are 2 state changes:

  • theading.Thread._bootstrap_inner() adds the thread into _active and then removes it from _active
  • theading.Thread._set_sentinel() creates a lock registers a callback which release the lock, so .join() can wait until the internal Python thread state is released for real

Currently, _shutdown() rely on _active. But for Python finalization: Py_Finalize() and Py_EndInterpreter(), we really want to wait until the Python thread state get delete, not only when theading.Thread._bootstrap_inner() exits.

test_threads_join_2() demonstrates the bug: "Fatal Python error: Py_EndInterpreter: not the last thread".

pablogsal
pablogsal approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I checked locally also with the patch in https://bugs.python.org/file48409/py_finalize.patch to test test_finalization_shutdown manually.

Good catch! 🎉

@vstinner
Copy link
Member Author

@gpshead: Would yo mind to review the updated PR?

@vstinner
bpo-36402: Fix threading._shutdown() race condition
81b56f1 
Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.
@vstinner
Copy link
Member Author

I rebased my PR and squashed commits. I clarified comments and the NEWS enty to specify that only non-daemon threads are joined. I reviewed my own PR and it LGTM :-) Since it already two approvals, I'm now comfortable enough to merge it.

@vstinner vstinner merged commit 468e5fe into python:master Jun 12, 2019
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2019
@vstinner @miss-islington
bpo-36402: Fix threading._shutdown() race condition (pythonGH-13948)
9494323 
Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.
(cherry picked from commit 468e5fe)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
@bedevere-bot
Copy link

GH-14033 is a backport of this pull request to the 3.8 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Jun 12, 2019
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2019
@vstinner @miss-islington
bpo-36402: Fix threading._shutdown() race condition (pythonGH-13948)
04d1cb6 
Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.
(cherry picked from commit 468e5fe)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
@bedevere-bot
Copy link

GH-14034 is a backport of this pull request to the 3.7 branch.

@vstinner vstinner deleted the threading_shutdown branch June 12, 2019 23:30
This was referenced Jun 12, 2019
Closed
Closed
Closed
vstinner added a commit that referenced this pull request Jun 13, 2019
@vstinner
[3.8] bpo-36402: Fix threading._shutdown() race condition (GH-13948) (G…
e40a97a 
…H-14050)

* bpo-36402: Fix threading._shutdown() race condition (GH-13948)

Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.

(cherry picked from commit 468e5fe)

* bpo-36402: Fix threading.Thread._stop() (GH-14047)

Remove the _tstate_lock from _shutdown_locks, don't remove None.

(cherry picked from commit 6f75c87)
@miss-islington miss-islington mentioned this pull request Jun 13, 2019
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 13, 2019
@vstinner @miss-islington
[3.8] bpo-36402: Fix threading._shutdown() race condition (pythonGH-1…
247fb9d 
…3948) (pythonGH-14050)

* bpo-36402: Fix threading._shutdown() race condition (pythonGH-13948)

Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.

(cherry picked from commit 468e5fe)

* bpo-36402: Fix threading.Thread._stop() (pythonGH-14047)

Remove the _tstate_lock from _shutdown_locks, don't remove None.

(cherry picked from commit 6f75c87)
(cherry picked from commit e40a97a)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
vstinner added a commit that referenced this pull request Jun 13, 2019
@miss-islington @vstinner
bpo-36402: Fix threading._shutdown() race condition (GH-13948) (GH-14050
6eb2878 
) (GH-14054)

* bpo-36402: Fix threading._shutdown() race condition (GH-13948)

Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.

(cherry picked from commit 468e5fe)

* bpo-36402: Fix threading.Thread._stop() (GH-14047)

Remove the _tstate_lock from _shutdown_locks, don't remove None.

(cherry picked from commit 6f75c87)
(cherry picked from commit e40a97a)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@vstinner @lisroach
bpo-36402: Fix threading._shutdown() race condition (pythonGH-13948)
390a7c0 
Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.
@wmanley
Copy link

wmanley commented Oct 11, 2019

This caused a regression for people overriding Thread.join to implement custom thread interruption. See https://stackoverflow.com/questions/50486083/ending-non-daemon-threads-when-shutting-down-an-interactive-python-session

@vstinner
Copy link
Member Author

This caused a regression for people overriding Thread.join to implement custom thread interruption. See https://stackoverflow.com/questions/50486083/ending-non-daemon-threads-when-shutting-down-an-interactive-python-session

I guess that you are talking about this regression: https://bugs.python.org/issue37788

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@vstinner @DinoV
bpo-36402: Fix threading._shutdown() race condition (pythonGH-13948)
63437ca 
Fix a race condition at Python shutdown when waiting for threads.
Wait until the Python thread state of all non-daemon threads get
deleted (join all non-daemon threads), rather than just wait until
Python threads complete.

* Add threading._shutdown_locks: set of Thread._tstate_lock locks
  of non-daemon threads used by _shutdown() to wait until all Python
  thread states get deleted. See Thread._set_tstate_lock().
* Add also threading._shutdown_locks_lock to protect access to
  threading._shutdown_locks.
* Add test_finalization_shutdown() test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead gpshead requested changes

@pitrou pitrou pitrou approved these changes

@pablogsal pablogsal pablogsal approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@methane methane Awaiting requested review from methane

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

@tim-one tim-one Awaiting requested review from tim-one

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@vstinner @bedevere-bot @miss-islington @wmanley @gpshead @pitrou @pablogsal @the-knights-who-say-ni