Skip to content

Comments

bpo-33540: Add block_on_close attr to socketserver#6911

Merged
vstinner merged 5 commits intopython:masterfrom
vstinner:block_on_close
May 24, 2018
Merged

bpo-33540: Add block_on_close attr to socketserver#6911
vstinner merged 5 commits intopython:masterfrom
vstinner:block_on_close

Conversation

@vstinner
Copy link
Member

@vstinner 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 22:54
Add a new block_on_close class attribute to ForkingMixIn and
ThreadingMixIn classes of socketserver.
@vstinner
Copy link
Member Author

I rebased my PR.

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

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

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"


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

Copy link
Member

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

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

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

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

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

Choose a reason for hiding this comment

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

pre-3.7

@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 added 2 commits May 24, 2018 02:39
* old => pre-3.7
* remove an empty line
@vstinner
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@vstinner vstinner merged commit 453bd0b into python:master May 24, 2018
@vstinner vstinner deleted the block_on_close branch May 24, 2018 01:15
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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 <[email protected]>
@bedevere-bot
Copy link

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 <[email protected]>
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.

5 participants