Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 7, 2018

@pablogsal pablogsal requested a review from a team as a code owner September 7, 2018 21:50
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Sep 7, 2018
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

The overall change LGTM, but I have a few comments/requests.

Copy link
Member

Choose a reason for hiding this comment

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

You can use support.join_thread(self.thread) to raise an error if the thread cannot be joined in 30 seconds, rather than being stuck.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change then the rest of thread.joins of this module as there are plenty.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to rename the attribute to "self.thread_key".

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@pablogsal pablogsal merged commit 5b7a2cb into python:master Sep 7, 2018
@pablogsal pablogsal deleted the bpo34246 branch September 7, 2018 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants