classification
Title: socketserver.BaseServer._handle_request_noblock() doesn't shutdown request if verify_request is False
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: martin.panter, palaviv, python-dev
Priority: normal Keywords: patch

Created on 2016-02-08 16:58 by palaviv, last changed 2016-02-19 04:31 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
socketserver-shutdown-if-verify-false.patch palaviv, 2016-02-08 16:58 review
socketserver-shutdown-if-verify-false2.patch palaviv, 2016-02-08 17:20 review
socketserver-shutdown-if-verify-false3.patch palaviv, 2016-02-09 20:38 patch with test review
socketserver-shutdown-if-verify-false4.patch palaviv, 2016-02-15 16:24 patch with simplified test review
Messages (12)
msg259861 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-08 16:58
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.
msg259863 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-08 17:20
Had a small mistake in the previous patch (did not notice process_request) call shutdown_request.

fixed the patch
msg259923 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-09 10:41
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.
msg259953 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-09 20:38
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.
msg260263 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-14 05:12
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.
msg260317 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-15 16:24
I changed the test to just check that shutdown_request is called. Hope this is more clear then the previous test.
msg260348 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-16 05:38
Yes this patch looks pretty good, thanks
msg260450 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-18 11:54
New changeset 651a6d47bc78 by Martin Panter in branch '3.5':
Issue #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 #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 #26309: Shut down SocketServer request if verify_request() is false
https://hg.python.org/cpython/rev/e0fbd25f0b36
msg260452 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-18 11:58
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().
msg260496 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-19 01:48
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.
msg260498 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-19 02:43
New changeset cba717fa8e10 by Martin Panter in branch '2.7':
Issue #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 #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 #26309: Merge socketserver fix from 3.5
https://hg.python.org/cpython/rev/c791d57c8168
msg260504 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-19 04:31
The race depended on how whether serve_forever had a chance to hande the first request before the main thread told it to stop.
History
Date User Action Args
2016-02-19 04:31:23martin.pantersetstatus: open -> closed

messages: + msg260504
2016-02-19 02:43:37python-devsetmessages: + msg260498
2016-02-19 01:48:48martin.pantersetstatus: closed -> open

messages: + msg260496
2016-02-18 11:58:29martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg260452

stage: patch review -> resolved
2016-02-18 11:54:37python-devsetnosy: + python-dev
messages: + msg260450
2016-02-16 05:38:50martin.pantersetmessages: + msg260348
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
2016-02-15 16:24:37palavivsetfiles: + socketserver-shutdown-if-verify-false4.patch

messages: + msg260317
2016-02-14 05:12:54martin.pantersetmessages: + msg260263
2016-02-09 20:38:50palavivsetfiles: + socketserver-shutdown-if-verify-false3.patch

messages: + msg259953
2016-02-09 10:41:51martin.pantersetnosy: + martin.panter

messages: + msg259923
stage: patch review
2016-02-08 17:20:00palavivsetfiles: + socketserver-shutdown-if-verify-false2.patch

messages: + msg259863
2016-02-08 16:58:33palavivcreate