Skip to content

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

I rebased my PR.

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

Comment thread Doc/library/socketserver.rst Outdated
: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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread Doc/whatsnew/3.7.rst Outdated

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

Choose a reason for hiding this comment

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

done

Comment thread Doc/whatsnew/3.7.rst Outdated
* :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
Copy Markdown
Member

Choose a reason for hiding this comment

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

pre-3.7

Comment thread Doc/whatsnew/3.7.rst Outdated
* :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
Copy Markdown
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
Copy Markdown

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

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

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

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

@miss-islington
Copy link
Copy Markdown
Contributor

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

@miss-islington
Copy link
Copy Markdown
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
Copy Markdown

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