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

socket.type value changes after using settimeout() #65526

Closed
giampaolo opened this issue Apr 22, 2014 · 19 comments
Closed

socket.type value changes after using settimeout() #65526

giampaolo opened this issue Apr 22, 2014 · 19 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@giampaolo
Copy link
Contributor

BPO 21327
Nosy @pitrou, @vstinner, @giampaolo, @bitdancer, @lekma, @1st1, @nnja, @raulcd
Superseder
  • bpo-32331: Fix socket.type on OSes with SOCK_NONBLOCK
  • Files
  • issue21327.patch
  • 21327.2.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 2018-05-29.14:07:06.427>
    created_at = <Date 2014-04-22.11:55:07.417>
    labels = ['type-bug']
    title = 'socket.type value changes after using settimeout()'
    updated_at = <Date 2018-05-29.14:07:06.425>
    user = 'https://github.com/giampaolo'

    bugs.python.org fields:

    activity = <Date 2018-05-29.14:07:06.425>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-05-29.14:07:06.427>
    closer = 'yselivanov'
    components = []
    creation = <Date 2014-04-22.11:55:07.417>
    creator = 'giampaolo.rodola'
    dependencies = []
    files = ['39009', '39069']
    hgrepos = []
    issue_num = 21327
    keywords = ['patch']
    message_count = 19.0
    messages = ['216999', '217000', '217002', '217020', '217032', '240910', '240914', '240933', '240939', '240996', '240997', '241028', '241031', '241225', '241754', '242426', '242427', '242429', '318030']
    nosy_count = 10.0
    nosy_names = ['pitrou', 'vstinner', 'giampaolo.rodola', 'r.david.murray', 'lekma', 'nvetoshkin', 'neologix', 'yselivanov', 'nnja', 'raulcd']
    pr_nums = []
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = '32331'
    type = 'behavior'
    url = 'https://bugs.python.org/issue21327'
    versions = ['Python 3.3', 'Python 3.4', 'Python 3.5']

    @giampaolo
    Copy link
    Contributor Author

    >>> s = socket.socket()
    >>> s.type
    <SocketType.SOCK_STREAM: 1>
    >>> s.settimeout(2)
    >>> s.type
    2049
    >>> 

    I can reproduce this with Python 3.5, 3.4 and 3.3.
    2.7 is not affected.

    @giampaolo
    Copy link
    Contributor Author

    It seems this was introduced in bpo-7523 / revision 12442ac3f7dd.

    @giampaolo
    Copy link
    Contributor Author

    Generally speaking I think it's fine to have this behavior only if the socket object is instantiated like this:

    >>> s = socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK)
    >>> s.type
    2049

    ...but when it comes to using settimeout() I would not expect that to happen (it's not cross platform).
    Sounds reasonable?

    @pitrou
    Copy link
    Member

    pitrou commented Apr 22, 2014

    I think distinguishing between the two situations would make the code yet more complicated (and fragile). It's a bit unfortunate that the type attribute has become a kitchen sink for disparate pieces of configuration.

    The fact that you are the first to complain though it was introduced in 3.2 does not make it very compelling to change the current behaviour, IMHO.

    @vstinner
    Copy link
    Member

    I would prefer to add something to get the type without SOCK_NONBLOCK nor
    SOCK_CLOEXEC. So new feature can only be added to Python 3.5. For older
    Python versions, you can to filter manually, which is difficult because you
    have yo check if SOCK_NONBLOCK and/or SOCK_CLOEXEC are available. You have
    the same issue in the C language.

    Anyway, all sockets are now created with SOCK_CLOEXEC since Python 3.4
    because of the PEP-446 (cloexec).

    We can add a new .sock_type attribute. Or .type could be an object with
    attributes like: type (int/enum), cloexec, nonblock, etc.

    Or simply a short helper method can be added. Ex:
    socket.get_socket_type(sock) or socket.get_socket_type(sock.type).

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    @Haypo Would you expect the new function to return a tuple?

    i.e:
    >>> socket.get_socket_type(sock)
    (socket.SOCK_STREAM, socket.SOCK_NONBLOCK)
    >>> socket.get_socket_type(sock2)
    (socket.SOCK_STREAM,)

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    after conversation with @r.david.murray I understand that we only want to return the type, not all the flags, so the function will return just the socket.SOCK_STREAM or socket.SOCK_DGRAM ...

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    While reproducing it I've seen that this has been already solved:
    >>> sock = socket.socket(type=socket.SOCK_STREAM)
    >>> sock.type
    <SocketKind.SOCK_STREAM: 1>
    >>> sock.settimeout(2)
    >>> sock.type
    <SocketKind.SOCK_STREAM: 1>
    But the next is still not correct:
    >>> sock = socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK)
    >>> sock.type
    2049

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    Forget my previous comment. Done with Linux and had the initial behavior again:
    >>> s = socket.socket()
    >>> s.type
    <SocketType.SOCK_STREAM: 1>
    >>> s.settimeout(2)
    >>> s.type
    2049

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    Added patch that solves issue, but I am not sure this is what you expect. If you think this is the expected behavior I will add docs and tests:
    >>> sock = socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK)
    >>> sock.type
    2049
    >>> socket.get_socket_type(sock)
    <SocketKind.SOCK_STREAM: 1>
    >>> sock = socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK |socket.SOCK_CLOEXEC)
    >>> sock.type
    526337
    >>> socket.get_socket_type(sock)
    <SocketKind.SOCK_STREAM: 1>

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 14, 2015

    Added docs and test in the patch.

    @nnja
    Copy link
    Contributor

    nnja commented Apr 14, 2015

    Maybe it would be better if the type property on socket had this behavior, rather than creating a new method.

    @bitdancer
    Copy link
    Member

    If we were designing from scratch, that would be true. But we are stuck with the behavior that we have, and cannot change it because we need to preserve backward compatibility. This is why we need to make it a new method.

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 16, 2015

    Added new patch as per review comments.

    @raulcd
    Copy link
    Mannequin

    raulcd mannequin commented Apr 21, 2015

    ping, any comment on the patch that was provided during the PyCon 2015 sprints?

    @bitdancer
    Copy link
    Member

    The behavior of s.type has changed in 3.5: it now behaves like the proposed new method behaves. Looking at the source I can't figure out why this is so. It doesn't seem as though it should be, but this needs to be sorted out before we can proceed.

    @bitdancer
    Copy link
    Member

    Wait, I take it back. I must have run the test incorrectly.

    @bitdancer
    Copy link
    Member

    I added some review comments.

    Something I don't understand: if settimeout sets NONBLOCK, and CLOEXEC is always set now, why does s=socket.socket() yield something whose type is SOCK_STREAM, while settimeout causes type to change? I don't understand the relationship between SOCK_NONBLOCK and O_NONBLOCK, either.

    Whoever came up with this API in linux was crazy, if you ask me. On the other hand, having type return what it does is clearly correct, given that we are mirroring the C API. Having a new sock_type attribute that provides the value without the extra flags makes sense in that context, IMO.

    @1st1
    Copy link
    Member

    1st1 commented May 29, 2018

    This issue is resolved in 3.7: https://bugs.python.org/issue32331

    @1st1 1st1 closed this as completed May 29, 2018
    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label May 29, 2018
    @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
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants