| 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. |
|
|