Skip to content
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.

Sign up for GitHub

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

Jump to bottom

socketserver.BaseServer._handle_request_noblock() doesn't shutdown request if verify_request is False #70497

Closed
palaviv mannequin opened this issue Feb 8, 2016 · 12 comments
Closed

socketserver.BaseServer._handle_request_noblock() doesn't shutdown request if verify_request is False #70497

palaviv mannequin opened this issue Feb 8, 2016 · 12 comments
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@palaviv
Copy link
Mannequin

palaviv mannequin commented Feb 8, 2016

BPO 26309
Nosy @vadmium, @palaviv
Files
  • socketserver-shutdown-if-verify-false.patch
  • socketserver-shutdown-if-verify-false2.patch
  • socketserver-shutdown-if-verify-false3.patch: patch with test
  • socketserver-shutdown-if-verify-false4.patch: patch with simplified test
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2016-02-19.04:31:23.977>
created_at = <Date 2016-02-08.16:58:33.464>
labels = ['library', 'performance']
title = "socketserver.BaseServer._handle_request_noblock() doesn't shutdown request if verify_request is False"
updated_at = <Date 2016-02-19.04:31:23.975>
user = 'https://github.com/palaviv'

    bugs.python.org fields:

    activity = <Date 2016-02-19.04:31:23.975>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-02-19.04:31:23.977>
closer = 'martin.panter'
components = ['Library (Lib)']
creation = <Date 2016-02-08.16:58:33.464>
creator = 'palaviv'
dependencies = []
files = ['41855', '41856', '41874', '41930']
hgrepos = []
issue_num = 26309
keywords = ['patch']
message_count = 12.0
messages = ['259861', '259863', '259923', '259953', '260263', '260317', '260348', '260450', '260452', '260496', '260498', '260504']
nosy_count = 3.0
nosy_names = ['python-dev', 'martin.panter', 'palaviv']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue26309'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']
    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 8, 2016

    When socketserver.BaseServer.verify_request() return False then we do not call shutdown_request. If we will take the TCPServer as example we will call get_request thus calling socket.accept() and creating a new socket but we will not call shutdown_request to close the unused socket.

    @palaviv palaviv mannequin added stdlib Python modules in the Lib dir performance Performance or resource usage labels Feb 8, 2016
    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 8, 2016

    Had a small mistake in the previous patch (did not notice process_request) call shutdown_request.

    fixed the patch

    @vadmium
    Copy link
    Member

    vadmium commented Feb 9, 2016

    The patch looks good to me. Are you interested in writing a unit test for it? Having said that, it might be a tricky test to write.

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 9, 2016

    I added a test to check the specific bug.

    There seems to be no testing at all on the verify_request feature so I will create some in different issue in the future.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 14, 2016

    Thanks for the test, but it is a bit confusing to have the shutdown_request() method call finish_request() and actually do normal request handling. Maybe it would be better to not handle the request (and not test that a response is received), and just check that shutdown_request() is called or that the client socket is explicitly closed.

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 15, 2016

    I changed the test to just check that shutdown_request is called. Hope this is more clear then the previous test.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 16, 2016

    Yes this patch looks pretty good, thanks

    @vadmium vadmium changed the title socketserver.BaseServer._handle_request_noblock() don't shutdwon request if verify_request is False socketserver.BaseServer._handle_request_noblock() doesn't shutdown request if verify_request is False Feb 16, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 18, 2016

    New changeset 651a6d47bc78 by Martin Panter in branch '3.5':
    Issue bpo-26309: Shut down socketserver request if verify_request() is false
    https://hg.python.org/cpython/rev/651a6d47bc78

    New changeset 0768edf5878d by Martin Panter in branch 'default':
    Issue bpo-26309: Merge socketserver fix from 3.5
    https://hg.python.org/cpython/rev/0768edf5878d

    New changeset e0fbd25f0b36 by Martin Panter in branch '2.7':
    Issue bpo-26309: Shut down SocketServer request if verify_request() is false
    https://hg.python.org/cpython/rev/e0fbd25f0b36

    @vadmium
    Copy link
    Member

    vadmium commented Feb 18, 2016

    For the Python 2 version I had to make some small changes to the test. I used indirection instead of “nonlocal”, and replaced the super() call because the classes are apparently the wrong kind for super().

    @vadmium vadmium closed this as completed Feb 18, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Feb 19, 2016

    Some of the buildbots failed (e.g. <http://buildbot.python.org/all/builders/ARMv7%20Ubuntu%202.7/builds/1145/steps/test/logs/stdio\>). I think the server is run in a separate thread, and the problem is a race between shutdown_request() being called and run_server returning.

    @vadmium vadmium reopened this Feb 19, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 19, 2016

    New changeset cba717fa8e10 by Martin Panter in branch '2.7':
    Issue bpo-26309: Rewrite test in main thread and avoid race condition
    https://hg.python.org/cpython/rev/cba717fa8e10

    New changeset 537608bafa5a by Martin Panter in branch '3.5':
    Issue bpo-26309: Rewrite test in main thread and avoid race condition
    https://hg.python.org/cpython/rev/537608bafa5a

    New changeset c791d57c8168 by Martin Panter in branch 'default':
    Issue bpo-26309: Merge socketserver fix from 3.5
    https://hg.python.org/cpython/rev/c791d57c8168

    @vadmium
    Copy link
    Member

    vadmium commented Feb 19, 2016

    The race depended on how whether serve_forever had a chance to hande the first request before the main thread told it to stop.

    @vadmium vadmium closed this as completed Feb 19, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @vadmium