bpo-35017: Lib/socketserver, do not accept any request after shutdown#9952

beledouxdenis · 2018-10-18T12:00:55Z

Prior to this revision,
After the shutdown of a BaseServer,
the server accepted a last single request
if it was sent between the server socket polling
and the polling timeout.

This can be problematic for instance
for a server restart
for which you do not want to interrupt the service,
by not closing the listening socket during the restart.
One request failed because of this behavior.

Note that only one request failed,
following requests were not accepted, as expected.

https://bugs.python.org/issue35017

We solved this issue in odoo/odoo by overriding _handle_request_noblock, in the below revision:
odoo/odoo@dcb5641#diff-7c4c00ad10cee1a79d1ed0d8b96f0bfaR140

We would like to avoid having to do so, as this is not a method which is supposed to be overridden.

the-knights-who-say-ni · 2018-10-18T12:00:59Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst

tirkarthi · 2018-10-25T09:01:42Z

I think Victor might help here in reviewing this PR. Friendly ping @vstinner

Lib/socketserver.py

…ythonGH-9952) Prior to this revision, After the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected.

beledouxdenis · 2018-10-25T12:41:30Z

Thank you very much for your review @vstinner
I have updated the PR with your remarks.

This targets a bug in Python 3.6 and 3.7. Is there something additional to do for this revision to land in these Python releases or will it be back-ported later, potentially through another pull request ?

tirkarthi · 2018-10-25T12:55:56Z

@beledouxdenis Once the PR is merged and the reviewer adds "needs backport to 3.7" and "needs backport to 3.6" labels to the PR @miss-islington will cherry pick the commit and create relevant PRs to the branches. If there are any conflicts then you might need to create a PR manually fixing the conflicts during cherry picks. Given the change I think there will be no conflicts.

Relevant dev guide section : https://devguide.python.org/committing/?highlight=backport#backporting-changes-to-an-older-version

beledouxdenis · 2018-10-25T13:05:09Z

Thank you for the clarification !

tirkarthi · 2018-10-25T13:47:28Z

You're welcome :) I could see only one commit for the PR. In case you have force pushed changes it will make review of the changes between review comments little harder to notice unlike having separate commits. Core devs squash and merge all the commits in a PR as a single commit so for future contributions please push the changes as separate commits for reviews.

Thanks

Lib/test/test_socketserver.py

bedevere-bot · 2018-10-25T14:15:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

This test can fail on slow buildbots, because it relies on a time condition The unit test can be safely removed, the fix being quite trivial.

The news message now represents what the revision is changing regarding the behavior rather than describing the current unexpected behavior.

Lib/socketserver.py

miss-islington · 2018-10-26T13:46:19Z

Thanks @beledouxdenis for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-10-26T13:46:34Z

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

…H-9952) Prior to this revision, after the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. (cherry picked from commit 10cb376) Co-authored-by: Denis Ledoux <[email protected]>

bedevere-bot · 2018-10-26T13:46:45Z

GH-10126 is a backport of this pull request to the 3.6 branch.

miss-islington · 2018-10-26T13:46:49Z

Sorry, @beledouxdenis and @vstinner, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 10cb3760e8631a27f5db1e51b05494e29306c671 2.7

…H-9952) Prior to this revision, after the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. (cherry picked from commit 10cb376) Co-authored-by: Denis Ledoux <[email protected]>

vstinner · 2018-10-26T13:47:47Z

@beledouxdenis: Hum, the bot failed to create a backport for Python 2.7. Can you please try to do the backport manually? Use "cherry_picker 10cb376 2.7" or "git cherry-pick -x 10cb376".

…H-9952)" This reverts commit 10cb376.

beledouxdenis · 2018-10-26T13:53:18Z

I didn't expect the backport to 2.7, I am not even sure the bug is there. I initially targeted 3.6 and 3.7.

Will check anyway if this can be reproduced in 2.7 :).

Prior to this revision, after the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. (cherry picked from commit 10cb376) Co-authored-by: Denis Ledoux <[email protected]>

vstinner · 2018-10-26T14:17:12Z

I didn't expect the backport to 2.7, I am not even sure the bug is there.

The code is almost the same, so I'm sure that it has the bug:

while not self.__shutdown_request: # XXX: Consider using another file descriptor or # connecting to the socket to wake this up instead of # polling. Polling reduces our responsiveness to a # shutdown request and wastes cpu at all other times. r, w, e = _eintr_retry(select.select, [self], [], [], poll_interval) if self in r: self._handle_request_noblock()

The fix should be the same :-)

beledouxdenis · 2018-10-26T14:18:44Z

I came to the same conclusion. I can re-use my unit test after all :-).

…H-9952) Prior to this revision, after the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. (cherry picked from commit 10cb376)

bedevere-bot · 2018-10-26T14:42:39Z

GH-10129 is a backport of this pull request to the 2.7 branch.

GH-10129) Prior to this revision, after the shutdown of a `BaseServer`, the server accepted a last single request if it was sent between the server socket polling and the polling timeout. This can be problematic for instance for a server restart for which you do not want to interrupt the service, by not closing the listening socket during the restart. One request failed because of this behavior. Note that only one request failed, following requests were not accepted, as expected. (cherry picked from commit 10cb376)

The method override `_handle_request_noblock` is there to solve a bug in Python socketserver library which has been solved in - Python 3.6: python/cpython@8b1f52b, - Python 3.7: python/cpython@9080824, through PR python/cpython#9952. These revisions will be included in Python releases 3.6.8 and 3.7.2, and the override will then become useless. We therefore can remove the `_handle_request_noblock` override as soon as the Python 3 releases installed on operating systems supported by Odoo are above - 3.6.8 for Python 3.6 - 3.7.2 for Python 3.7

The method override `_handle_request_noblock` is there to solve a bug in Python socketserver library which has been solved in - Python 3.6: python/cpython@8b1f52b, - Python 3.7: python/cpython@9080824, through PR python/cpython#9952. These revisions will be included in Python releases 3.6.8 and 3.7.2, and the override will then become useless. We therefore can remove the `_handle_request_noblock` override as soon as the Python 3 releases installed on operating systems supported by Odoo are above - 3.6.8 for Python 3.6 - 3.7.2 for Python 3.7 closes#29706

the-knights-who-say-ni added the CLA not signed label Oct 18, 2018

bedevere-bot added the awaiting review label Oct 18, 2018

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 18, 2018

beledouxdenis force-pushed the fix-issue-35017 branch from 43af5d2 to cf649e1Compare October 19, 2018 07:35

tirkarthi reviewed Oct 20, 2018
View reviewed changes

Misc/NEWS.d/next/Library/2018-10-19-09-32-07.bpo-35017.vAanVd.rst Outdated Show resolvedHide resolved

vstinner reviewed Oct 25, 2018
View reviewed changes

Lib/socketserver.py Outdated Show resolvedHide resolved

beledouxdenis force-pushed the fix-issue-35017 branch from cf649e1 to a9c4abeCompare October 25, 2018 10:13

beledouxdenis force-pushed the fix-issue-35017 branch from a9c4abe to 57f0822Compare October 25, 2018 12:00

vstinner requested changes Oct 25, 2018
View reviewed changes

Lib/test/test_socketserver.py Outdated Show resolvedHide resolved

bedevere-bot added awaiting changes and removed awaiting review labels Oct 25, 2018

beledouxdenis added 2 commits October 26, 2018 00:02

bpo-35017: remove the unit test as it's time-based
928be37
This test can fail on slow buildbots, because it relies on a time condition The unit test can be safely removed, the fix being quite trivial.

bpo-35017: reword the news to represent the new behavior
b9fb425
The news message now represents what the revision is changing regarding the behavior rather than describing the current unexpected behavior.

vstinner reviewed Oct 25, 2018
View reviewed changes

Lib/socketserver.py Outdated Show resolvedHide resolved

bpo-35017: reword inline comment to be shorter and cleaner
7992fb0

vstinner added needs backport to 3.6 labels Oct 26, 2018

vstinner approved these changes Oct 26, 2018
View reviewed changes

bedevere-bot removed the awaiting changes label Oct 26, 2018

bedevere-bot added the awaiting merge label Oct 26, 2018

vstinner added the needs backport to 2.7 label Oct 26, 2018

vstinner merged commit 10cb376 into python:masterOct 26, 2018

bedevere-bot removed the awaiting merge label Oct 26, 2018

bedevere-bot removed the needs backport to 3.7 label Oct 26, 2018

bedevere-bot removed the needs backport to 3.6 label Oct 26, 2018

miss-islington assigned vstinner Oct 26, 2018

vstinner added a commit that referenced this pull request Oct 26, 2018
Revert "bpo-35017, socketserver: don't accept request after shutdown (G…
a854bfa
…H-9952)" This reverts commit 10cb376.

bedevere-bot removed the needs backport to 2.7 label Oct 26, 2018

beledouxdenis mentioned this pull request Dec 21, 2018
[TODO] service/server: note to remove override that will become useless odoo/odoo#29706
Closed

Uh oh!

bpo-35017: Lib/socketserver, do not accept any request after shutdown#9952

bpo-35017: Lib/socketserver, do not accept any request after shutdown #9952

Uh oh!

Conversation

beledouxdenis commented Oct 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Oct 18, 2018

Uh oh!

Uh oh!

tirkarthi commented Oct 25, 2018

Uh oh!

Uh oh!

beledouxdenis commented Oct 25, 2018

Uh oh!

tirkarthi commented Oct 25, 2018

Uh oh!

beledouxdenis commented Oct 25, 2018

Uh oh!

tirkarthi commented Oct 25, 2018

Uh oh!

Uh oh!

bedevere-bot commented Oct 25, 2018

Uh oh!

Uh oh!

miss-islington commented Oct 26, 2018

Uh oh!

bedevere-bot commented Oct 26, 2018

Uh oh!

bedevere-bot commented Oct 26, 2018

Uh oh!

miss-islington commented Oct 26, 2018

Uh oh!

vstinner commented Oct 26, 2018

Uh oh!

beledouxdenis commented Oct 26, 2018

Uh oh!

vstinner commented Oct 26, 2018

Uh oh!

beledouxdenis commented Oct 26, 2018

Uh oh!

bedevere-bot commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

beledouxdenis commented Oct 18, 2018•
edited
Loading