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

Context manager of socket.socket is not documented #69099

Closed
desbma mannequin opened this issue Aug 21, 2015 · 9 comments
Closed

Context manager of socket.socket is not documented #69099

desbma mannequin opened this issue Aug 21, 2015 · 9 comments
Labels
docs Documentation in the Doc dir easy type-feature A feature request or enhancement

Comments

@desbma
Copy link
Mannequin

desbma mannequin commented Aug 21, 2015

BPO 24911
Nosy @vstinner, @berkerpeksag, @vadmium, @desbma, @1st1
Files
  • socket-context.patch
  • socket-context.v2.patch
  • 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-24.06:23:24.902>
    created_at = <Date 2015-08-21.20:34:06.173>
    labels = ['easy', 'type-feature', 'docs']
    title = 'Context manager of socket.socket is not documented'
    updated_at = <Date 2016-04-24.06:23:24.902>
    user = 'https://github.com/desbma'

    bugs.python.org fields:

    activity = <Date 2016-04-24.06:23:24.902>
    actor = 'martin.panter'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2016-04-24.06:23:24.902>
    closer = 'martin.panter'
    components = ['Documentation']
    creation = <Date 2015-08-21.20:34:06.173>
    creator = 'desbma'
    dependencies = []
    files = ['40307', '41943']
    hgrepos = []
    issue_num = 24911
    keywords = ['patch', 'easy']
    message_count = 9.0
    messages = ['248979', '249172', '249381', '260358', '260363', '260393', '260419', '262840', '264094']
    nosy_count = 7.0
    nosy_names = ['vstinner', 'docs@python', 'python-dev', 'berker.peksag', 'martin.panter', 'desbma', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24911'
    versions = ['Python 3.5', 'Python 3.6']

    @desbma
    Copy link
    Mannequin Author

    desbma mannequin commented Aug 21, 2015

    socket.socket has a context manager to automatically close the socket with the with statement: https://hg.python.org/cpython/file/d1bf181afa82/Lib/socket.py#l138

    However it is not documented, unlike socket.create_connection.

    @desbma desbma mannequin assigned docspython Aug 21, 2015
    @desbma desbma mannequin added the docs Documentation in the Doc dir label Aug 21, 2015
    @berkerpeksag berkerpeksag added easy type-feature A feature request or enhancement labels Aug 25, 2015
    @vadmium
    Copy link
    Member

    vadmium commented Aug 26, 2015

    IMO the change notice for create_connection() should be moved to apply to the “socket” class, perhaps the “Socket Objects” section. bpo-9794 looks like context manager support was added specifically for create_connection(), however any socket object, no matter what function is used to create it, should be usable the same way for free.

    Also, we should explicitly document that exiting the context manager (“with” statement) invokes close(), since that affects how the underlying socket is closed when makefile() has been used.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 31, 2015

    Here is my proposed patch

    @berkerpeksag
    Copy link
    Member

    socket-context.patch looks good to me. There is no need to add a NEWS entry for this.

    @1st1
    Copy link
    Member

    1st1 commented Feb 16, 2016

    The patch looks good. It would also be cool if you can add a short code snippet somewhere:

       sock = socket.socket()
       with sock:
          sock.bind(('127.0.0.1', 8080))
          sock.listen()
          ...

    @vstinner
    Copy link
    Member

    I reviewed the patch.

    It would also be cool if you can add a short code snippet somewhere:

    The socket module has examples.

    Why not modifying these examples to promote the context manager protocol?

    Example:
    --------

    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s
    # use with to ensure that the socket is closed, especially on error
    with s:
      s.bind((HOST, PORT))
      s.listen(1)
      conn, addr = s.accept()
      with conn:
        print('Connected by', addr)
        while True:
            data = conn.recv(1024)
            if not data: break
            conn.sendall(data)
        conn.close()

    The second "with conn:" is maybe overkill. What do you think?

    For a client connection, usually I prefer to explicitly close the socket (even if I use "with conn:") to get exception on my ".close()" line, instead of getting an exception from the context manager, which is harder to understand.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 18, 2016

    Thanks for the reviews.

    In this new patch, I modified two existing examples, but did not add any new example. Does that work for you Yury?

    Also modified example code for the socketserver module.

    Victor: IMO the “with conn” in the example is not overkill. It ensures the client connection socket is cleaned up, which is completely independent of the server listening socket s.

    What exceptions can you get out of conn.close()? I can only think of unusual programming errors like EBADF. I would prefer to remove close() as being redundant with the context manager.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 4, 2016

    FWIW I discovered that socket.close() or __exit__() does not actually raise exceptions for cases like EBADF, see bpo-26685.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 24, 2016

    New changeset d5f7980dd654 by Martin Panter in branch '3.5':
    Issue bpo-24911: All socket objects are context managers; update examples
    https://hg.python.org/cpython/rev/d5f7980dd654

    New changeset 711201953505 by Martin Panter in branch 'default':
    Issue bpo-24911: Merge socket context manager doc from 3.5
    https://hg.python.org/cpython/rev/711201953505

    @vadmium vadmium closed this as completed Apr 24, 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
    docs Documentation in the Doc dir easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants