Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

socketserver context manager #70592

Closed
palaviv mannequin opened this issue Feb 21, 2016 · 12 comments
Closed

socketserver context manager #70592

palaviv mannequin opened this issue Feb 21, 2016 · 12 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@palaviv
Copy link
Mannequin

palaviv mannequin commented Feb 21, 2016

BPO 26404
Nosy @terryjreedy, @vadmium, @palaviv
Files
  • socketserver_context_manager.patch
  • socketserver_context_manager2.patch: patch after first review
  • socketserver_context_manager3.patch: patch after second review
  • socketserver_context_manager4.patch: patch after third review
  • 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-04-13.04:00:06.249>
    created_at = <Date 2016-02-21.20:55:10.233>
    labels = ['type-feature', 'library']
    title = 'socketserver context manager'
    updated_at = <Date 2016-04-13.04:00:06.248>
    user = 'https://github.com/palaviv'

    bugs.python.org fields:

    activity = <Date 2016-04-13.04:00:06.248>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-04-13.04:00:06.249>
    closer = 'martin.panter'
    components = ['Library (Lib)']
    creation = <Date 2016-02-21.20:55:10.233>
    creator = 'palaviv'
    dependencies = []
    files = ['41993', '42008', '42016', '42030']
    hgrepos = []
    issue_num = 26404
    keywords = ['patch']
    message_count = 12.0
    messages = ['260639', '260670', '260696', '260757', '260771', '260871', '260935', '261008', '261029', '261040', '263298', '263303']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'python-dev', 'martin.panter', 'palaviv']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26404'
    versions = ['Python 3.6']

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 21, 2016

    As Martin commented on my patch at bpo-26392 the socketserver.server_close is like the file close. That made me think that we should add a context manager to the socketserver.

    @palaviv palaviv mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Feb 21, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Feb 22, 2016

    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.

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 22, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 24, 2016

    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.

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 24, 2016

    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.

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 25, 2016

    updated the patch according to the CR comments.

    @terryjreedy
    Copy link
    Member

    terryjreedy commented Feb 27, 2016

    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?

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 29, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 29, 2016

    Server_close() was only documentated last year; see bpo-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.

    @terryjreedy
    Copy link
    Member

    terryjreedy commented Mar 1, 2016

    I agree and consider this is an argument for adding the context manager methods and using them with 'with' in the examples.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2016

    New changeset 5c4303c46a18 by Martin Panter in branch 'default':
    Issue bpo-26404: Add context manager to socketserver, by Aviv Palivoda
    https://hg.python.org/cpython/rev/5c4303c46a18

    @vadmium
    Copy link
    Member

    vadmium commented Apr 13, 2016

    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.

    @vadmium vadmium closed this as completed Apr 13, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants