Skip to content

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

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:threading_shutdown
Jun 12, 2019
Merged

bpo-36402: Fix threading._shutdown() race condition#13948
vstinner merged 1 commit intopython:masterfrom
vstinner:threading_shutdown

Conversation

@vstinner
Copy link
Copy Markdown
Member

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

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
Copy Markdown
Member Author

vstinner commented Jun 10, 2019

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
Copy Markdown
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
Copy Markdown
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.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 11, 2019

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

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Jun 11, 2019

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 reviewed Jun 11, 2019
View reviewed changes
pablogsal
pablogsal approved these changes Jun 11, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

vstinner commented Jun 12, 2019

@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
Copy Markdown
Member Author

vstinner commented Jun 12, 2019

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
Copy Markdown
Contributor

miss-islington commented Jun 12, 2019

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
Copy Markdown

bedevere-bot commented Jun 12, 2019

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

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
Copy Markdown

bedevere-bot commented Jun 12, 2019

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
Copy Markdown

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
Copy Markdown
Member Author

vstinner 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

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.

8 participants

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