classification
Title: socketserver context manager
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: martin.panter, palaviv, python-dev, terry.reedy
Priority: normal Keywords: patch

Created on 2016-02-21 20:55 by palaviv, last changed 2016-04-13 04:00 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
socketserver_context_manager.patch palaviv, 2016-02-21 20:55 review
socketserver_context_manager2.patch palaviv, 2016-02-22 18:38 patch after first review review
socketserver_context_manager3.patch palaviv, 2016-02-24 09:02 patch after second review review
socketserver_context_manager4.patch palaviv, 2016-02-25 17:26 patch after third review review
Messages (12)
msg260639 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-21 20:55
As Martin commented on my patch at issue 26392 the socketserver.server_close is like the file close. That made me think that we should add a context manager to the socketserver.
msg260670 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-22 10:56
I would support this change, as long as we aren’t supporting using server_close() to stop the server loop at the same time.

It might be worth updating other example code that uses server_close(), in particular in the xmlrpc.server documentation.
msg260696 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-22 18:38
Only closing the server :).
1. Did the changes requested in the CR.
2. Changed the example's in xmlrpc.server, http.server to use context manager.
3. Changed the xmlrpc.server, http.server server implementation when running python -m {xmlrpc.server, http.server} to use context manager.
msg260757 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-24 01:59
Thanks, the patch looks pretty good to me. I left some comments about minor things. Also, someone will need to write a What’s New entry.
msg260771 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-24 09:02
Updated the patch:
1. Did the changes requested in the CR.
2. Changed the example's in wsgiref.simple_server to use context manager.
3. Added What's New entry.
msg260871 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-25 17:26
updated the patch according to the CR comments.
msg260935 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-02-27 09:37
This seems like an appropriate enhancement.  I notice that the serve_forever examples did not previously have server_close().  Was this an oversight or will the server_close in __exit__ just be a no-op?
msg261008 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-29 14:17
The examples did not close the server but I am not sure if that was a mistake. The server is not being closed only on examples where it is expected to run until there is a keyboard interrupt. However you can see that when you send keyboard interrupt to a server you start using python -m http.server you will do close the server.
msg261029 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-29 22:54
Server_close() was only documentated last year; see Issue 23254. For the examples that run until you interrupt them, the servers currently emit a resource warning (in addition to the KeyboardInterrupt traceback and the Python 2 bytes warnings):

$ python -bWall TCPServer.py
127.0.0.1 wrote:
TCPServer.py:16: BytesWarning: str() on a bytes instance
  print(self.data)
b'hello world with TCP'
127.0.0.1 wrote:
TCPServer.py:16: BytesWarning: str() on a bytes instance
  print(self.data)
b'python is nice'
^CTraceback (most recent call last):
  File "TCPServer.py", line 28, in <module>
    server.serve_forever()
  File "/usr/lib/python3.5/socketserver.py", line 237, in serve_forever
    ready = selector.select(poll_interval)
  File "/usr/lib/python3.5/selectors.py", line 367, in select
    fd_event_list = self._poll.poll(timeout)
KeyboardInterrupt
sys:1: ResourceWarning: unclosed <socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 9999)>
[Exit 1]

If you ignore the warning, there isn’t much effective difference, because the socket gets closed when the process exits, or if you are lucky, when Python garbage collects the global “server” object. But IMO it is bad practice not to clean up resources properly, especially in an API example.
msg261040 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2016-03-01 06:37
I agree and consider this is an argument for adding the context manager methods and using them with 'with' in the examples.
msg263298 - (view) Author: Roundup Robot (python-dev) Date: 2016-04-13 02:41
New changeset 5c4303c46a18 by Martin Panter in branch 'default':
Issue #26404: Add context manager to socketserver, by Aviv Palivoda
https://hg.python.org/cpython/rev/5c4303c46a18
msg263303 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-13 04:00
Thanks for the patch Aviv. I made a few minor English grammar etc tweaks in the version I committed, as pointed out in the review comments.
History
Date User Action Args
2016-04-13 04:00:06martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg263303

stage: patch review -> resolved
2016-04-13 02:41:30python-devsetnosy: + python-dev
messages: + msg263298
2016-03-01 06:37:00terry.reedysetmessages: + msg261040
2016-02-29 22:54:51martin.pantersetmessages: + msg261029
2016-02-29 14:17:57palavivsetmessages: + msg261008
2016-02-27 09:37:39terry.reedysetnosy: + terry.reedy
messages: + msg260935
2016-02-25 17:26:50palavivsetfiles: + socketserver_context_manager4.patch

messages: + msg260871
2016-02-24 09:02:59palavivsetfiles: + socketserver_context_manager3.patch

messages: + msg260771
2016-02-24 01:59:14martin.pantersetmessages: + msg260757
stage: patch review
2016-02-22 18:38:50palavivsetfiles: + socketserver_context_manager2.patch

messages: + msg260696
2016-02-22 10:56:22martin.pantersetmessages: + msg260670
2016-02-21 20:55:10palavivcreate