Issue26309
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) * | 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) * | 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) * | Date: 2016-02-16 05:38 | |
Yes this patch looks pretty good, thanks |
|||
| msg260450 - (view) | Author: Roundup Robot (python-dev) | 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) * | 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) * | 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) | 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) * | 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:23 | martin.panter | set | status: open -> closed messages: + msg260504 |
| 2016-02-19 02:43:37 | python-dev | set | messages: + msg260498 |
| 2016-02-19 01:48:48 | martin.panter | set | status: closed -> open messages: + msg260496 |
| 2016-02-18 11:58:29 | martin.panter | set | status: open -> closed resolution: fixed messages: + msg260452 stage: patch review -> resolved |
| 2016-02-18 11:54:37 | python-dev | set | nosy:
+ python-dev messages: + msg260450 |
| 2016-02-16 05:38:50 | martin.panter | set | messages:
+ 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:37 | palaviv | set | files:
+ socketserver-shutdown-if-verify-false4.patch messages: + msg260317 |
| 2016-02-14 05:12:54 | martin.panter | set | messages: + msg260263 |
| 2016-02-09 20:38:50 | palaviv | set | files:
+ socketserver-shutdown-if-verify-false3.patch messages: + msg259953 |
| 2016-02-09 10:41:51 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg259923 |
| 2016-02-08 17:20:00 | palaviv | set | files:
+ socketserver-shutdown-if-verify-false2.patch messages: + msg259863 |
| 2016-02-08 16:58:33 | palaviv | create | |