[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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-33540: Add block_on_close attr to socketserver #6911

Merged
merged 5 commits into from May 24, 2018

Conversation

Copy link
Member

vstinner commented May 16, 2018

Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver.

https://bugs.python.org/issue33540

vstinner added 2 commits May 22, 2018
Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver.
Copy link
Member Author

vstinner commented May 22, 2018

I rebased my PR.

Copy link
Member

ned-deily left a comment

A few wording nits. Otherwise, LGTM


:meth:`socketserver.ThreadingMixIn.server_close` waits until all non-daemon
threads complete. Use daemonic threads by setting
threads complete, except if
:attr:`socketserver.ThreadingMixIn.block_on_close` attribute if false. Use
Copy link
Member

ned-deily May 23, 2018

Choose a reason for hiding this comment

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

should be "attribute is false"

@@ -129,6 +132,9 @@ server classes.
:meth:`socketserver.ThreadingMixIn.server_close` now waits until all
child processes and non-daemonic threads complete.

Add a new :attr:`socketserver.ForkingMixIn.block_on_close` class
attribute to opt-in for the old behaviour.

Copy link
Member

ned-deily May 23, 2018

Choose a reason for hiding this comment

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

It might read better it the addition were not a separate paragraph. How about removing the blank line and rewording a bit, perhaps:

If necessary, set the new :attr:socketserver.ForkingMixIn.block_on_close class
attribute to False to obtain the pre-3.7 behavior.

Copy link
Member Author

vstinner May 24, 2018

Choose a reason for hiding this comment

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

Your proposed sentence doesn't explicitly say that the attribute is new in 3.7. I removed the empty line.


Add a new :attr:`socketserver.ForkingMixIn.block_on_close` class attribute to
:class:`socketserver.ForkingMixIn` and :class:`socketserver.ThreadingMixIn`
classes. Set the class attribute to ``False`` to get the old behaviour.
Copy link
Member

ned-deily May 23, 2018

Choose a reason for hiding this comment

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

old -> pre-3.7

Copy link
Member Author

vstinner May 24, 2018

Choose a reason for hiding this comment

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

done

* :meth:`socketserver.ThreadingMixIn.server_close` now waits until all
non-daemon threads complete. Set the new
:attr:`socketserver.ThreadingMixIn.block_on_close` class attribute to
``False`` to get the old behaviour.
Copy link
Member

ned-deily May 23, 2018

Choose a reason for hiding this comment

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

pre-3.7

* :meth:`socketserver.ForkingMixIn.server_close` now waits until all
child processes complete. Set the new
:attr:`socketserver.ForkingMixIn.block_on_close` class attribute to ``False``
to get the old behaviour.
Copy link
Member

ned-deily May 23, 2018

Choose a reason for hiding this comment

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

pre-3.7

Copy link

bedevere-bot commented May 23, 2018

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

vstinner added 2 commits May 24, 2018
* old => pre-3.7
* remove an empty line
Copy link
Member Author

vstinner commented May 24, 2018

I have made the requested changes; please review again.

Copy link

bedevere-bot commented May 24, 2018

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

vstinner merged commit 453bd0b into python:master May 24, 2018
vstinner deleted the block_on_close branch May 24, 2018
Copy link
Contributor

miss-islington commented May 24, 2018

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

Copy link
Contributor

miss-islington commented May 24, 2018

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

Copy link
Contributor

miss-islington commented May 24, 2018

Sorry, @vstinner, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 453bd0bc65b7ea6a18c43da69143ab10d54c0a35 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 24, 2018
Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver to opt-in for pre-3.7 behaviour.
(cherry picked from commit 453bd0b)

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

bedevere-bot commented May 24, 2018

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

miss-islington added a commit that referenced this pull request May 24, 2018
Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver to opt-in for pre-3.7 behaviour.
(cherry picked from commit 453bd0b)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver to opt-in for pre-3.7 behaviour.
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.

None yet

5 participants