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

[palaviv]

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]

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]

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

fixed the patch

[vadmium]

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]

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]

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]

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

[vadmium]

Yes this patch looks pretty good, thanks

[python-dev]

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]

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]

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.