Skip to content

bpo-30357 each test in test_thread waits until all spawned threads finish - #1583

Merged
vstinner merged 4 commits into
python:2.7from
grzgrzgrz3:fix-issue-30357
May 15, 2017
Merged

bpo-30357 each test in test_thread waits until all spawned threads finish#1583
vstinner merged 4 commits into
python:2.7from
grzgrzgrz3:fix-issue-30357

Conversation

@grzgrzgrz3

@grzgrzgrz3grzgrzgrz3 commented May 14, 2017

Copy link
Copy Markdown
Contributor

No description provided.

vstinner
vstinner reviewed May 15, 2017
View reviewed changes
Comment threadLib/test/test_thread.py Outdated
self.next_ident = 0
self.thread_count = thread._count()

def tearDown(self):

@vstinnervstinnerMay 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This code looks similar to threading_cleanup(): please use threading_setup() and threading_cleanup() from test.support.

@vstinner

vstinner commented May 15, 2017

Copy link
Copy Markdown
Member

LGTM, but I would prefer to fix experiment this change in the master branch, before touching the super-stable 2.7 branch. Can you propose the same change for the master branch please?

@grzgrzgrz3

grzgrzgrz3 commented May 15, 2017

Copy link
Copy Markdown
ContributorAuthor

I was unable to reproduce this issue on the master branch. However still it's worth to clean up tests correctly. I will create new PR for master branch.

What you think about rewriting test_save_exception_state_on_error and use _thread._set_sentinel instead thread._count?

@vstinner

vstinner commented May 15, 2017

Copy link
Copy Markdown
Member

You also need to add yourself to Misc/ACKS. You might also document the change in Misc/NEWS. I created #1592 for master where I credited you ;-)

@vstinner

vstinner commented May 15, 2017

Copy link
Copy Markdown
Member

I was unable to reproduce this issue on the master branch. However still it's worth to clean up tests correctly.

Well, we are talking about race conditions. It's really hard to reproduce them. It's likely that the test order changed between Python 2.7 and master, but it doesn't mean that master is bugfix. Just that the bug is less likely :-)

I will create new PR for master branch.

Sorry, I wanted to fix quickly the bug, so I pushed a fix based on your work on the master branch and added your name ;-) Can you please try to backport the fix to 2.7? Try to cherry-pick the change from master (or complete this change).

grzgrzgrz3and others added 2 commits May 15, 2017 18:36
@grzgrzgrz3@vstinner
bpo-30357: test_thread now uses threading_cleanup() (python#1592)
f83a06f
test_thread: setUp() now uses support.threading_setup() and
support.threading_cleanup() to wait until threads complete to avoid
random side effects on following tests.
Co-Authored-By: Victor Stinner <victor.stinner@gmail.com>
@grzgrzgrz3@vstinner
bpo-30357: test_thread now uses threading_cleanup() (python#1592)
596d8bd
test_thread: setUp() now uses support.threading_setup() and
support.threading_cleanup() to wait until threads complete to avoid
random side effects on following tests.
Co-Authored-By: Victor Stinner <victor.stinner@gmail.com>
@vstinner
vstinner merged commit 6924ed5 into python:2.7May 15, 2017
@vstinner

vstinner commented May 15, 2017

Copy link
Copy Markdown
Member

LGTM! Thanks for the backport.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@grzgrzgrz3@vstinner@the-knights-who-say-ni