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

asyncio.create_unix_server has an off-by-one error concerning the backlog parameter #90871

Open
jnsnow mannequin opened this issue Feb 10, 2022 · 4 comments
Open

asyncio.create_unix_server has an off-by-one error concerning the backlog parameter #90871

jnsnow mannequin opened this issue Feb 10, 2022 · 4 comments
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@jnsnow
Copy link
Mannequin

jnsnow mannequin commented Feb 10, 2022

BPO 46715
Nosy @asvetlov, @1st1, @jnsnow
Files
  • issue.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 = None
    created_at = <Date 2022-02-10.21:24:35.963>
    labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', '3.7', 'expert-asyncio']
    title = 'asyncio.create_unix_server has an off-by-one error concerning the backlog parameter'
    updated_at = <Date 2022-02-10.21:24:35.963>
    user = 'https://github.com/jnsnow'

    bugs.python.org fields:

    activity = <Date 2022-02-10.21:24:35.963>
    actor = 'jnsnow'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['asyncio']
    creation = <Date 2022-02-10.21:24:35.963>
    creator = 'jnsnow'
    dependencies = []
    files = ['50618']
    hgrepos = []
    issue_num = 46715
    keywords = ['patch']
    message_count = 1.0
    messages = ['413025']
    nosy_count = 3.0
    nosy_names = ['asvetlov', 'yselivanov', 'jnsnow']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46715'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @jnsnow
    Copy link
    Mannequin Author

    jnsnow mannequin commented Feb 10, 2022

    Hi, asyncio.create_unix_server appears to treat the "backlog" parameter as where 0 means that *no connection will ever possibly be pending*, which (at the very least for UNIX sockets on my machine) is untrue.

    Consider a (non-asyncio) server:

    import os, socket, sys, time
    
    sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
    sock.bind('test.sock')
    
    sock.listen(backlog=0)
    
    while True:
        print('.', end='', file=sys.stderr)
        time.sleep(1)

    This server never calls accept(), and uses a backlog of zero. However, a client can actually still successfully call connect against such a server:

    import os, socket, time
    
    sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
    sock.setblocking(False)
    
    sock.connect('test.sock')
    print("Connected!")

    When run against the server example, the first invocation of this client will actually connect successfully (Surprising, but that's how the C syscalls work too, so... alright) but the second invocation of this client will raise BlockingIOError (EAGAIN).

    Further, if we amend the first server example to actually call accept(), it will succeed when the first client connects -- demonstrating that the actual total queue length here was actually effectively 1, not 0.

    (i.e. there's always room for at least one connection to be considered, and the backlog counts everybody else.)

    However, in asyncio.BaseSelectorEventLoop._accept_connection(...), the code uses for _ in range(backlog) to determine the maximum number of accept calls to make. When backlog is set to zero, this means we will never call accept, even when there are pending connections.

    Note that when backlog=1, this actually allows for *two* pending connections before clients are rejected, but this loop will only fire once. This behavior is surprising, because backlog==0 means we'll accept no clients, but backlog==1 means we will allow for two to enqueue before accepting both. There is seemingly no way with asyncio to actually specify "Exactly one pending connection".

    I think this loop should be amended to reflect the actual truth of the backlog parameter, and it should iterate over backlog + 1. This does necessitate a change to [Lib/test/test_asyncio/test_selector_events.py](https://github.com/python/cpython/blob/main/Lib/test/test_asyncio/test_selector_events.py) which believes that backlog=100 means that accept() should be called 100 times (instead of 101.)

    A (very) simple fix is attached here; if it seems sound, I can spin a real PR on GitHub.

    @jnsnow jnsnow mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Feb 10, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @kumaraditya303 kumaraditya303 removed 3.11 only security fixes 3.10 only security fixes 3.9 only security fixes 3.8 only security fixes 3.7 (EOL) end of life labels Oct 18, 2022
    @gvanrossum
    Copy link
    Member

    Sure. Could you send a PR? Ideally it would include a test for the new behavior and a docs update.

    Does this apply only to UNIX sockets, or also to INET sockets?

    @jnsnow
    Copy link

    jnsnow commented May 15, 2023

    Sure. Could you send a PR? Ideally it would include a test for the new behavior and a docs update.

    I'll give it a shot. I haven't submitted anything to the project before so it might take me a handful of minutes to figure out how to tie and/or untie my shoelaces.

    Does this apply only to UNIX sockets, or also to INET sockets?

    I'll be honest, I don't know. I'll have to experiment. I don't know if there are cross-platform gotchas at play here between Windows/macOS/BSD/Linux. socket programming is, famously, very easy and nobody ever gets it wrong. :)

    @gvanrossum
    Copy link
    Member

    I would start with a test -- such a test should be easily reused for INET sockets, so then you can test that case, and we will find out in CI.

    While it's not authoritative, the FreeBSD developers handbook has this clear sentence: "[t]he backlog variable tells sockets how many incoming requests to accept while you are busy processing the last request."

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    Status: Todo
    Development

    No branches or pull requests

    3 participants